-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
fix!: Prevent accidental use of insecure key sizes & misconfiguration of secrets #852
fix!: Prevent accidental use of insecure key sizes & misconfiguration of secrets #852
Conversation
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.
Looks like we have missing coverage for node-16 and 18.
ERROR: Coverage for branches (94.72%) does not meet global threshold (95%)
However the changes in general look fine to me 👍 think we just need to find where that missing coverage is and add the tests.
verify.js
Outdated
} else if (secretOrPublicKey.asymmetricKeyType === 'rsa') { | ||
options.algorithms = RSA_KEY_ALGS | ||
} else { | ||
options.algorithms = PUB_KEY_ALGS | ||
} |
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.
This is a slight change from previous behaviour: if we had a PEM with BEGIN RSA PUBLIC KEY
, we knew we could narrow down to RSA only, otherwise for BEGIN PUBLIC KEY
we allow all public algs.
The former case could have asymmetricKeyType === 'pub-rsa'
so we end up allowing slightly more than before.
IMO let's pick up RSA-PSS here, and also handle ECC while we're here for consistency (technically a breaking change for EC though so we need to document it):
if (secretOrPublicKey.type === 'secret') {
options.algorithms = HS_ALGS;
} else if (secretOrPublicKey.asymmetricKeyType === 'rsa-pss' || secretOrPublicKey.asymmetricKeyType === 'rsa-pss') {
options.algorithms = RSA_KEY_ALGS;
} if (secretOrPublicKey.asymmetricKeyType === 'ec') {
options.algorithms = EC_KEY_ALGS;
}
else {
options.algorithms = PUB_KEY_ALGS;
}
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.
It'd be great to add some explanation of this in the README as well - we don't specify what happens if algorithms
is undefined
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.
Done. We'll add the change to our migration guide
Added checks to prevent invalid secrets from being used with the HS*** algorithms when signing and verifying Added checks to prevent the use of insecure asymmetric key sizes except when explicitly overriden via options BREAKING CHANGE: Requires node 12.x or later to allow use of `KeyObject`
Addressing review comments
Rebasing and Addressing review comments
BREAKING CHANGE: Requires node 12.x or later to allow use of
KeyObject
By submitting a PR to this repository, you agree to the terms within the Auth0 Code of Conduct. Please see the contributing guidelines for how to create and submit a high-quality PR for this repo.
Description
This PR covers addresses 3 misconfigurations which could compromise security.
allowInsecureKeySizes
totrue
in the options object.Buffer
s containing malicious objects from being used as key material.Testing
New automated tests have been added for each scenario.
Checklist