4 ms·

Author here: I was curious what HN would think of this token style, we're currently using it for getseam.com . I think it has a lot of advantages over other tok

Author here: I was curious what HN would think of this token style, we're currently using it for getseam.com . I think it has a lot of advantages over other token styles (e.g. double-click to select, standard alphabet, compatible with secret scanning). Although this is a typescript library, we originally designed this API key for use in Ruby on Rails and the pattern should be fairly portable.

joshuawarner32 4y ago
On iPhone the long-press-to-select doesn’t cross the underscore boundary, so you end up just selecting part of the token. Maybe you can do without those underscores?
- __sy__ 4y ago
  **shakes fist at sky/apple**
koolba 4y ago
Couple comments upon looking at the actual code: - You can promisify randomBytes once and reuse it rather than twice for every invocation - There shouldn’t be a default value for the company name or people will end up using it. - The company name isn’t validated so it could contain underscores which would cause issues with the short token parsing as it assumes it’s the second “chunk” - The equals comparison of the hashes for the secrets is not timing safe. It’s not as bad as if they were plain text but it does short circuit due to how string equals works. Use the actual built in timing safe equals on the Buffer hash (not the stringified hex).
- seveibar 4y ago
  thanks for taking a look! I've created an issue on the repo and should be able to address these tonight :)
jffry 4y ago
Thanks for sharing! As somebody who has been bitten before by copy-pasting a key I appreciate that as the headlining benefit :) Thoughts from an ergonomic perspective: - Base58 is nice and makes easy-to-handle values which is nice - Naming of shortToken, longToken, and token are confusing to me, because it makes it sound like all of these values are tokens/secrets of some sort, rather than components of the token. Not to bikeshed, but what jumps to my mind is more like "prefix_id_secret", which helps make it clear what role each one plays. - I don't think keyPrefix should have a default, I think your function should raise an error if it's not provided. Otherwise more than one user of your library is going to end up with "mycompany" keys in the wild. - You probably ought to validate that keyPrefix does not contain an underscore and raise an exception if it does, otherwise that way lies pain for people trying to handle these keys. Thoughts from a security perspective: - You should probably use crypto.timingSafeEquals(buffer, buffer) [1] instead of comparing strings - If you're trying to store the secret's hash, isn't something like bcrypt/scrypt preferred over raw sha256 these days? [1] https://nodejs.org/api/crypto.html#cryptotimingsafeequala-b https://nodejs.org/api/crypto.html#cryptotimingsafeequala-b
- koolba 4y ago
  > If you're trying to store the secret's hash, isn't something like bcrypt/scrypt preferred over raw sha256 these days? Not for this type of use case. If a secret is long and generated from a cryptographically secure source (eg /dev/urandom), then any cryptographic hash function is fine as brute forcing the secret itself is not feasible. A single pass of sha256 is fine and presumably you’d be doing many of these operations each second in a high throughput application. “Slow” hash functions like bcrypt or scrypt are for user provided secrets that might not have a large amount of entropy. It’s fine for something like user authentication but would be way too slow and pointless for API keys.
  - benatkin 4y ago
    > If a secret is long It seems this one, at 24 base-58 characters, is only roughly twice as long as it needs to be, right? How short is too short?
- 4y ago
remram 4y ago
If you are prefixing it, why not prefix it with a full domain name? mycompany_com_BRTRKFsL_51FwqftsmMDHHbJAMEXXHCgG is not much longer than mycompany_BRTRKFsL_51FwqftsmMDHHbJAMEXXHCgG and allows reporting leaked secrets (for example using the GitHub scheme [1]) without having to look up the URL in some sort of registry, e.g. POST https://mycompany.com/.well-known/report-leaked-secrets https://mycompany.com/.well-known/report-leaked-secrets [1]: https://docs.github.com/en/developers/overview/secret-scanning-partner-program#create-a-secret-alert-service https://docs.github.com/en/developers/overview/secret-scanni... edit: See also the discussion from last time: https://news.ycombinator.com/item?id=28296864 https://news.ycombinator.com/item?id=28296864
- seveibar 4y ago
  I think this is an excellent idea! However, we didn't like the aesthetic of the version that included the domain- I'll admit it's not great reasoning.
adastra22 4y ago
Don’t use base58. Even bitcoin, which originated or at least popularized base58 has moved away from it. They are a pain to encode and decode, with a naive implementation requiring a bignum library, and they don’t save you much in the end over a base32.
- epse 4y ago
  But you don't need to encode or decode anything here? Tokens are just random strings?
  - adastra22 4y ago
    How do you think the token is generated if not through an encoding process? Other disadvantage I forgot to mention earlier: base58 is variable length, which is a foot gun that will bite you eventually.
    - formerly_proven 4y ago
      Using base58 here strikes me as simply unnecessary. You could just use the base58 alphabet and directly generate random strings in that alphabet, easily and cheaply. The only thing you loose is the ability to decode into a bit shorter binary string for storage - almost certainly not worth it.
splix 4y ago
Can you tell more about the prefix? Not very clear to me. Is it issuer id or user id? And if it's later and is user provided, how it helps with scanning
- bluehatbrit 4y ago
  It's the issuers company or product name. GitHub for example prefix their tokens with "gh_" which makes it easy to scan for tokens uploaded to repos or even across the web if they wanted. GitHub has secret scanning features built in now, with a prefix of your product name or company name you could easily create a regex of sorts to find when someone has uploaded an API key for your application to their GitHub repo and revoke the token or email them.
mdaniel 4y ago
I don't follow what the security property is of involving sha256 against the b58 encoded value? If it serves as a checksum against tpyoss, wouldn't one wish to include the shortToken also? Otherwise, it's just as much attacker controlled content as the b58 version is, as best I can tell As an implementation observation, I am also on a life-long campaign to rid the world of `split(...)[-1]` type manipulations, because they lack the context of a more rigorous "parsing" style. In this specific case, it allows attackers to smuggle almost arbitrary characters between the shortToken and longToken: checkAPIKey("alpha_BRTRKFsL_and this one time at band camp\r\nContent-Length: 0\r\n_51FwqftsmMDHHbJAMEXXHCgG", "d70d981d87b449c107327c2a2afbf00d4b58070d6ba571aac35d7ea3e7c79f37") Also for your consideration, the code currently has duplication: export const extractLongTokenHash = (token: string) => hashLongToken(extractLongToken(token)) // ...snip... longTokenHash: hashLongToken(extractLongToken(token)), // ...snip... ) => hashLongToken(extractLongToken(token)) === expectedLongTokenHash and my experience is that it's so easy to remember to update one and forget to update the others
- seveibar 4y ago
  Thanks, yes I agree the ".split(_)[-1]" is ugly and there should be validation on the key prior to operations on it! Making an issue :)

joshuawarner32 4y ago

On iPhone the long-press-to-select doesn’t cross the underscore boundary, so you end up just selecting part of the token. Maybe you can do without those underscores?

__sy__ 4y ago
**shakes fist at sky/apple**

koolba 4y ago

Couple comments upon looking at the actual code: - You can promisify randomBytes once and reuse it rather than twice for every invocation - There shouldn’t be a default value for the company name or people will end up using it. - The company name isn’t validated so it could contain underscores which would cause issues with the short token parsing as it assumes it’s the second “chunk” - The equals comparison of the hashes for the secrets is not timing safe. It’s not as bad as if they were plain text but it does short circuit due to how string equals works. Use the actual built in timing safe equals on the Buffer hash (not the stringified hex).

seveibar 4y ago
thanks for taking a look! I've created an issue on the repo and should be able to address these tonight :)

jffry 4y ago

Thanks for sharing! As somebody who has been bitten before by copy-pasting a key I appreciate that as the headlining benefit :) Thoughts from an ergonomic perspective: - Base58 is nice and makes easy-to-handle values which is nice - Naming of shortToken, longToken, and token are confusing to me, because it makes it sound like all of these values are tokens/secrets of some sort, rather than components of the token. Not to bikeshed, but what jumps to my mind is more like "prefix_id_secret", which helps make it clear what role each one plays. - I don't think keyPrefix should have a default, I think your function should raise an error if it's not provided. Otherwise more than one user of your library is going to end up with "mycompany" keys in the wild. - You probably ought to validate that keyPrefix does not contain an underscore and raise an exception if it does, otherwise that way lies pain for people trying to handle these keys. Thoughts from a security perspective: - You should probably use crypto.timingSafeEquals(buffer, buffer) [1] instead of comparing strings - If you're trying to store the secret's hash, isn't something like bcrypt/scrypt preferred over raw sha256 these days? [1] https://nodejs.org/api/crypto.html#cryptotimingsafeequala-b https://nodejs.org/api/crypto.html#cryptotimingsafeequala-b

koolba 4y ago
> If you're trying to store the secret's hash, isn't something like bcrypt/scrypt preferred over raw sha256 these days? Not for this type of use case. If a secret is long and generated from a cryptographically secure source (eg /dev/urandom), then any cryptographic hash function is fine as brute forcing the secret itself is not feasible. A single pass of sha256 is fine and presumably you’d be doing many of these operations each second in a high throughput application. “Slow” hash functions like bcrypt or scrypt are for user provided secrets that might not have a large amount of entropy. It’s fine for something like user authentication but would be way too slow and pointless for API keys.
- benatkin 4y ago
  > If a secret is long It seems this one, at 24 base-58 characters, is only roughly twice as long as it needs to be, right? How short is too short?
4y ago

remram 4y ago

If you are prefixing it, why not prefix it with a full domain name? mycompany_com_BRTRKFsL_51FwqftsmMDHHbJAMEXXHCgG is not much longer than mycompany_BRTRKFsL_51FwqftsmMDHHbJAMEXXHCgG and allows reporting leaked secrets (for example using the GitHub scheme [1]) without having to look up the URL in some sort of registry, e.g. POST https://mycompany.com/.well-known/report-leaked-secrets https://mycompany.com/.well-known/report-leaked-secrets [1]: https://docs.github.com/en/developers/overview/secret-scanning-partner-program#create-a-secret-alert-service https://docs.github.com/en/developers/overview/secret-scanni... edit: See also the discussion from last time: https://news.ycombinator.com/item?id=28296864 https://news.ycombinator.com/item?id=28296864

seveibar 4y ago
I think this is an excellent idea! However, we didn't like the aesthetic of the version that included the domain- I'll admit it's not great reasoning.

adastra22 4y ago

Don’t use base58. Even bitcoin, which originated or at least popularized base58 has moved away from it. They are a pain to encode and decode, with a naive implementation requiring a bignum library, and they don’t save you much in the end over a base32.

epse 4y ago
But you don't need to encode or decode anything here? Tokens are just random strings?
- adastra22 4y ago
  How do you think the token is generated if not through an encoding process? Other disadvantage I forgot to mention earlier: base58 is variable length, which is a foot gun that will bite you eventually.
  - formerly_proven 4y ago
    Using base58 here strikes me as simply unnecessary. You could just use the base58 alphabet and directly generate random strings in that alphabet, easily and cheaply. The only thing you loose is the ability to decode into a bit shorter binary string for storage - almost certainly not worth it.

splix 4y ago

Can you tell more about the prefix? Not very clear to me. Is it issuer id or user id? And if it's later and is user provided, how it helps with scanning

bluehatbrit 4y ago
It's the issuers company or product name. GitHub for example prefix their tokens with "gh_" which makes it easy to scan for tokens uploaded to repos or even across the web if they wanted. GitHub has secret scanning features built in now, with a prefix of your product name or company name you could easily create a regex of sorts to find when someone has uploaded an API key for your application to their GitHub repo and revoke the token or email them.

mdaniel 4y ago

I don't follow what the security property is of involving sha256 against the b58 encoded value? If it serves as a checksum against tpyoss, wouldn't one wish to include the shortToken also? Otherwise, it's just as much attacker controlled content as the b58 version is, as best I can tell As an implementation observation, I am also on a life-long campaign to rid the world of `split(...)[-1]` type manipulations, because they lack the context of a more rigorous "parsing" style. In this specific case, it allows attackers to smuggle almost arbitrary characters between the shortToken and longToken: checkAPIKey("alpha_BRTRKFsL_and this one time at band camp\r\nContent-Length: 0\r\n_51FwqftsmMDHHbJAMEXXHCgG", "d70d981d87b449c107327c2a2afbf00d4b58070d6ba571aac35d7ea3e7c79f37") Also for your consideration, the code currently has duplication: export const extractLongTokenHash = (token: string) => hashLongToken(extractLongToken(token)) // ...snip... longTokenHash: hashLongToken(extractLongToken(token)), // ...snip... ) => hashLongToken(extractLongToken(token)) === expectedLongTokenHash and my experience is that it's so easy to remember to update one and forget to update the others

seveibar 4y ago
Thanks, yes I agree the ".split(_)[-1]" is ugly and there should be validation on the key prior to operations on it! Making an issue :)