hmac Verify and Sign should allow key to be of type string

[dillonstreator]

Verify and Sign accepts an interface{} for the key. It's unexpected that string is not a valid input.

[oxisto]

I would definitely prefer this over #157, but for me it is still a little bit unclear why the need for the string support arrises in the first place. As mentioned in the other PR, we try to support the key format that the underlying encryption / hashing algorithm needs. In this case, the hmac package needs a ' []byte', which makes sense since this key should ideally be a random number of bytes and not reassemble anything that is human readable, such as a password (where a string could make sense).

[dillonstreator]

it is still a little bit unclear why the need for the string support arrises in the first place

@oxisto It's purely for being friendlier to the caller. Without a breaking change, the hmac Verify/Sign interface can't type the input as []byte. Ideally, Verify/Sign for hmac signing methods would type the key input to []byte.

I don't think anyone is questioning whether or not the underlying hmac package should be accepting []byte.

Callers that were passing string to Verify/Sign and running into this type error are still most likely doing this exact same string -> []byte conversion in the calling code.

If strings were an uncommon datatype for storing keys, this PR and #157 probably wouldn't exist and I would fully understand the hesitation to introduce this conversion into this package.

[oxisto]

it is still a little bit unclear why the need for the string support arrises in the first place

@oxisto It's purely for being friendlier to the caller. Without a breaking change, the hmac Verify/Sign interface can't type the input as []byte. Ideally, Verify/Sign for hmac signing methods would type the key input to []byte.

I don't think anyone is questioning whether or not the underlying hmac package should be accepting []byte.

Callers that were passing string to Verify/Sign and running into this type error are still most likely doing this exact same string -> []byte conversion in the calling code.

If strings were an uncommon datatype for storing keys, this PR and #157 probably wouldn't exist and I would fully understand the hesitation to introduce this conversion into this package.

Sorry for being so opinionated here - well after all that is was the Go community is based on ;) I get the caller friendliness aspect, I still don't get how you even end up with a key as a string. Would you mind sharing some of the scenario in which you are calling this library with a string key?

My fear is a little bit that this makes it even "easier" going into a (in my opinion) wrong road of having a human readable string as a key like string s = "changeme". Symmetric key based JWTs are really tricky to use securely from the start and I don't think that this helps to be honest.

Other opinions? @mfridman @Waterdrips @ripienaar

[oxisto]

BTW this (my) view seems to be shared by other JWT libraries as well. See https://github.com/lestrrat-go/jwx/blob/9348cb779184bd6b63a2a0422dfdaf787db9c3bc/jwk/jwk.go#L35-L44. Only []byte is supported for symmetric keys.

[dillonstreator]

I'm loading the signing key from the environment and it's being stored as a string. This seems like a fairly common pattern that I've seen.

Ah I hadn't reviewed what other libraries are doing but this gives me better perspective.

My fear is a little bit that this makes it even "easier" going into a (in my opinion) wrong road of having a human readable string as a key like string s = "changeme".

I don't think that if this library allowed strings as input it would somehow suggest callers should be hardcoding keys in source and/or exposing them. []byte may not be as easily readable as a plain string but it can still be converted to a string so is the idea security by obscurity?

keyBytes := []byte{99, 104, 97, 110, 103, 101, 109, 101}
keyStr := "changeme"

I'd be worried if I saw someone doing either of these in source ^

Also I appreciate differing opinions and the learning opportunities they present :)

[oxisto]

I'm loading the signing key from the environment and it's being stored as a string. This seems like a fairly common pattern that I've seen.

Ah I hadn't reviewed what other libraries are doing but this gives me better perspective.

My fear is a little bit that this makes it even "easier" going into a (in my opinion) wrong road of having a human readable string as a key like string s = "changeme".

I don't think that if this library allowed strings as input it would somehow suggest callers should be hardcoding keys in source and/or exposing them. []byte may not be as easily readable as a plain string but it can still be converted to a string so is the idea security by obscurity?

No, quite the contrary. The issue with string-based keys is that you are effectively limiting yourself to "human readable" characters in the string, because as you mentioned in the scenario are using that key in an environment variable and you have a hard time entering/storing "non-ASCII" characters on the console, so your key is probably something like "mdfs123asd$ instead of something generated by crypto/rand.Read. That limits the maximum entropy of the key.

What you "should" do is only use base64 encoded version of the key in the environment (as a string) and use []byte internally (using base64.Decode). This way you can use the maximum entropy of byte to generate a random key.

Also I appreciate differing opinions and the learning opportunities they present :)

Even better of course would be to avoid symmetric based JWTs altogether and use something like ECDSA-based JWTs with a JSON Web Key Set to verify for your clients.

[dillonstreator]

@oxisto this is very informative. Thank you for taking the time to explain.

I wonder if users that lack this knowledge would resort to continuing to use string keys to begin with after running into this type error (as I did). Effectively this change would just make it easier for users to continuing following bad practices even though they're probably still following them regardless.

Do you think there is value in having this reasoning documented on this repo? Maybe even a reference in code next to the Sign/Verify methods? IMO it would be great if even one other person was able to have these same learnings..

Either way, I feel a lot less strongly about this library supporting the callers ability to follow a bad practice.

[oxisto]

@oxisto this is very informative. Thank you for taking the time to explain.

I wonder if users that lack this knowledge would resort to continuing to use string keys to begin with after running into this type error (as I did). Effectively this change would just make it easier for users to continuing following bad practices even though they're probably still following them regardless.

Do you think there is value in having this reasoning documented on this repo? Maybe even a reference in code next to the Sign/Verify methods? IMO it would be great if even one other person was able to have these same learnings..

Either way, I feel a lot less strongly about this library supporting the callers ability to follow a bad practice.

Documenting this reasoning in the functions is a good idea. Would you be open to try to do that?

[dillonstreator]

hey @oxisto i opened this pr to add some code documentation notes around this #249
curious for your feedback