-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Conversation
cookie-secret is not encoded, it is used as-is. It is true that most users will pass a string of bytes which are not uniformly distributed across all byte values, but rather only include convenient printable ascii values. If the key is long enough, that's actually OK, there's still enough entropy in the string. If you just use the ascii characters typically used for urlsafe base64: You could actually specify any arbitrary bytes if you really wanted ... $ COOKIE_SECRET="$(dd if=/dev/urandom bs=16 count=1 2>/dev/null)"
$ printf "%s" "$COOKIE_SECRET" | hexdump -C
00000000 36 50 3f b8 2f 7d 38 4f c3 94 f9 18 ba f3 f0 4d |6P?./}8O.......M|
00000010
$ oauth2_proxy --cookie-secret="$COOKIE_SECRET" ... see also http://security.stackexchange.com/questions/111532/about-the-aes-algorithm-key-and-ciphertext |
Ok, that makes sense if the cookie secret is being treated as a raw byte string. It may be worth treating the cookie secret as a base64 or base16 encoded value so all possible byte values can be represented. Using printable ascii only gives access to 37% of the possible values for each byte. It is correct that allowing 64 possible byte values in a 16 byte string allows for many combinations:
But it is orders of magnitude smaller than what it could be:
Even using all 95 printable ascii characters is orders of magnitude smaller:
Is using a standard encoding like base16 or base64 for the cookie secret in the configuration text something you would be open to? Perhaps in a pull request? This is important for my use case because running several instances of oauth2_proxy behind a load balancer, each instance needs to have the same secret. The configuration management systems we use don't support the use of raw byte strings and I don't think I'll be allowed to put it into "production" for our internal tools if we can't specify truly random secrets. |
Maybe a 24-character secret key would satisfy you, with another couple orders of magnitude:
I should also clarify my "arbitrary bytes" tip above to say that I think zero values aka null bytes will be problematic. Also, there are more convenient ways of specifying specific arbitrary non-null bytes: COOKIE_SECRET=$(printf "\x0F\xE2...")
COOKIE_SECRET=$(python -c "print('0FE2...'.decode('hex'))")
oauth2_proxy --cookie-secret="$COOKIE_SECRET" ...
# or inline
oauth2_proxy --cookie-secret="$(printf '\x0F\xE2...')" ... You can likely put hex in your config management system and somehow get it decoded for the cmdline arg or env var. The documentation could probably be improved to give people a hint about this :) |
OK. I'm reading this as a polite "no thanks" for the idea of base64 encoding the cookie secret so you can represent a 32 byte string in printable characters with this many possible values:
I appreciate your consideration of the suggestion :) |
In case there is any interest later, here is a diff: master...ntrepid8:feature/base64-encoded-cookie-secret |
For the record, I don't think accepting base64 is necessarily a bad idea, nor am I a maintainer of this project. It would need a new option so the current one remains backwards compatible (which your branch does correctly), so I was clarifying how the current option works and what is possible with it. Someone in a better position than me can judge whether it's worth it. |
@ntrepid8 I'm going to re-open this. Seems totally fair to conditionally treat values as base64 encoded and automatically base64decode if the value consists of that character set. I'd accept such a PR That would allow us to document the expected (ideal) format of that field as base64. There is a small chance such a change would be backwards incompatible for some users, but i think we can version appropriately to handle that. |
also, thanks @ploxiln for clarifying some of the options to put raw bytes in the env variables |
@jehiah do you prefer pull requests with all the commits flattened in to a single commit? Also, to clarify do you want me to consolidate the cookie-secret to a single option (rather than cookie-secret and cookie-secret-b64) and treat it as a base64 encoded value unless it's in an incompatible format? And in the case of an incompatible format, treat it as raw bytes? If that's the case, I'll submit a PR with those changes. |
I don't have a preference for commit format for review, but I generally ask for squashing to a single commit before merging. Yup. exactly. if it looks like base64, treat it as such, otherwise treat it as raw bytes. (and document base64 as the preferred format) |
e9fd935
to
c38447c
Compare
c38447c
to
cdebfd6
Compare
func secretBytes(secret string) []byte { | ||
b, err := base64.URLEncoding.DecodeString(addPadding(secret)) | ||
if err == nil { | ||
return []byte(addPadding(string(b))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why =
pad the binary result of base64 decoding?
What sort of encoding is expected for the cookie secret value? It looks like the validate test is counting the number of characters (16, 24, or 32) in the string but I'm not sure how that relates to the number of bytes represented by the string.
If the encoding was urlsafe base64 a 32 byte string would look like this:
This string is 44 characters with the padding. Each character represents 6 bits so we need all the bits from the first 42 characters and then 4 bits from the 43rd character, and then the rest is padding.
How are we getting 32 characters == 32 bytes?