Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds HeaderSHA256 predicate #1905

Merged
merged 1 commit into from
Dec 8, 2022
Merged

Conversation

AlexanderYastrebov
Copy link
Member

@AlexanderYastrebov AlexanderYastrebov commented Nov 22, 2021

HeaderSHA256 predicate matches SHA-256 hash of the configured header value.

go test ./predicates/auth/ -run NONE -bench BenchmarkHeaderSHA256Match -benchmem
goos: linux
goarch: amd64
pkg: github.com/zalando/skipper/predicates/auth
cpu: Intel(R) Core(TM) i5-8350U CPU @ 1.70GHz
BenchmarkHeaderSHA256Match-8     4675351               237.2 ns/op             0 B/op          0 allocs/op

Signed-off-by: Alexander Yastrebov [email protected]

@AlexanderYastrebov AlexanderYastrebov marked this pull request as draft November 22, 2021 00:57
Comment on lines +371 to +390
Hash values only hide secrets from parties that have access to the source of Skipper routes.
Authentication strength depends on the strength of the secret value so e.g.
`HeaderSHA256("X-Secret", "2bb80d537b1da3e38bd30361aa855686bde0eacd7162fef6a25fe97bf527a25b")`
is not stronger than just `Header("X-Secret", "secret")`.
Copy link
Member Author

@AlexanderYastrebov AlexanderYastrebov Nov 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

discuss: add salt?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added guide on how to generate secure header value.
I think salt is not required if secret value is random, its length is at least 32 bytes (SHA-256 output size) and there is no need to hide the possible reuse of the same secret value in several routes.

In general proper salt value has its own requirements so the idea is to get rid of it if possible to minimize number of parameters that users need to specify and understand.

@szuecs
Copy link
Member

szuecs commented Nov 22, 2021

What's the use case?
It sounds more like an auth filter to me. A predicate will slow down all route lookoups, that's why we do not validate Jwt* predicates.
I think you want to do something independent of the chosen algorithm. What if SHA256 is broken and you need to use $someOtheHashFunction? I would suggest passing the chosen hash function as parameter to the filter.
On the other hand what's the difference between this and basic auth? Maybe better to make a better basic auth filter, that do not need a file but can have the salted,hashed secret as filter parameter?

@AlexanderYastrebov
Copy link
Member Author

What's the use case?

Match routes with known header value without revealing the actual value to the parties that have access to the routes source (source code, ingress definitions, allowed to query /routes endpoint)

A predicate will slow down all route lookoups

The benchmark shows that it takes 340 ns to match header hash.

I think you want to do something independent of the chosen algorithm. What if SHA256 is broken and you need to use $someOtheHashFunction? I would suggest passing the chosen hash function as parameter to the filter.

If that happens we will introduce HeaderSomeOtheHashFunction and discourage use of HeaderSHA256.

On the other hand what's the difference between this and basic auth? Maybe better to make a better basic auth filter, that do not need a file but can have the salted,hashed secret as filter parameter?

This predicate allows users to implement Basic Authentication as well as shown in the docs without revealing the username:password pair.

### HeaderSHA256

Matches if SHA-256 hash of the header value (secret) equals to any of the configured hash values.
Several hash values could be used to match multiple secrets e.g. during secret rotation.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same could be achieved using two routes with two different HeaderSHA256 but it is more convenient to use single predicate with multiple secrets to avoid route duplication, e.g. in zalando.org/skipper-predicate ingress annotation.

@AlexanderYastrebov AlexanderYastrebov force-pushed the HeaderSHA256-predicate branch 2 times, most recently from 4a43994 to d533822 Compare November 22, 2021 17:16
@AlexanderYastrebov AlexanderYastrebov force-pushed the HeaderSHA256-predicate branch 2 times, most recently from 58e9404 to b2c4473 Compare December 7, 2022 17:28
@AlexanderYastrebov AlexanderYastrebov marked this pull request as ready for review December 7, 2022 17:29
@szuecs
Copy link
Member

szuecs commented Dec 7, 2022

👍

`HeaderSHA256` predicate matches SHA-256 hash of the configured header value.

```
go test ./predicates/auth/ -run NONE -bench BenchmarkHeaderSHA256Match -benchmem
goos: linux
goarch: amd64
pkg: github.com/zalando/skipper/predicates/auth
cpu: Intel(R) Core(TM) i5-8350U CPU @ 1.70GHz
BenchmarkHeaderSHA256Match-8     4675351               237.2 ns/op             0 B/op          0 allocs/op
```

Signed-off-by: Alexander Yastrebov <[email protected]>
@szuecs
Copy link
Member

szuecs commented Dec 8, 2022

👍

1 similar comment
@AlexanderYastrebov
Copy link
Member Author

👍

@AlexanderYastrebov AlexanderYastrebov merged commit c9260a4 into master Dec 8, 2022
@AlexanderYastrebov AlexanderYastrebov deleted the HeaderSHA256-predicate branch December 8, 2022 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants