jwt: calculate kid from public key #174
Conversation
WalkthroughThis pull request modifies the JWT verification and signing processes in the crypto utility module. It simplifies import statements and enhances variable naming for clarity in the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant R as Resign Function
participant D as DER Encoder
participant H as SHA256 Hasher
C->>R: Call resign(jwt, private_key)
R->>R: Construct SubjectPublicKeyInfo from RSA key
R->>D: Encode public key info to DER
D-->>R: Return DER bytes
R->>H: Compute SHA-256 hash of DER bytes
H-->>R: Return hash (kid)
R->>C: Return new signed JWT with updated kid
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: omertuc The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/cluster_crypto/crypto_utils/jwt.rs (1)
94-111: Robust rework of KID calculation.This block correctly constructs and DER-encodes the
SubjectPublicKeyInfobefore hashing it to compute the KID. It aligns well with Kubernetes’ recommended approach, improving interoperability. A few considerations:
- Error handling: Although the code properly handles encoding and hashing errors, consider verifying that
public_key_data()never returns invalid data (e.g., empty).- Readability: For clarity, consider extracting the DER-encoding (lines 102–109) into a helper function.
- Testing: It would be beneficial to include a test that validates the derived KID matches Kubernetes expectations.
Would you like a helper function or test stub to ensure consistent KID generation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/cluster_crypto/crypto_utils/jwt.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
src/cluster_crypto/crypto_utils/jwt.rs (2)
7-7: Import looks good.The introduction of
BitStringfrombcderis necessary for constructing the subject public key info.
9-9: Appropriate new imports.Adding
rfc5280,InMemorySigningKeyPair,KeyAlgorithm, andSignaligns with the need to structure and sign the public key info.
|
/hold writing a test |
|
/unhold wrote the test |
# Background Recert resigns JWTs found in the cluster. The kid field in the Kubernetes JWT header is a Base64URL encoded hash of the public key used to sign the JWT. The kid used by verifiers (e.g. kube-apiserver) to quickly identify the key used to sign the JWT, instead of brute-forcing. # Issue Likely to solve issues.redhat.com/browse/OCPBUGS-49972 The current implementation uses the SHA256 hash of the private key to calculate the kid. This is inconsistent with the Kubernetes implementation [1] which uses the public key to calculate the kid. In recent versions of Kubernetes, a kid mismatch leads to "unauthorized" errors, which eventually leads to a collapse of the cluster. # Solution This commit changes the kid calculation to use the SHA256 hash of the public key instead of the private key, so that it becomes more consistent with Kubernetes # Backwards compatibility Seeds processed with older versions of recert that don't contain this fix should still work if "rejuvenated" by newer versions of recert which do contain this fix. This is because the new version will simply overwrite the `kid` field with the correct value. # References [1] https://github.com/openshift/kubernetes/blob/6fdacf04117cef54a0babd0945e8ef87d0f9461d/pkg/serviceaccount/jwt.go#L92-L112 Signed-off-by: Omer Tuchfeld <omer@tuchfeld.dev>
|
/unhold |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/cluster_crypto/crypto_utils/jwt.rs (1)
98-98: Fix typo in error message.The error message contains a typo: "headern not objecT" should be "header not object".
- .context("headern not objecT")? + .context("header not object")?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/cluster_crypto/crypto_utils.rs(4 hunks)src/cluster_crypto/crypto_utils/jwt.rs(3 hunks)
🔇 Additional comments (3)
src/cluster_crypto/crypto_utils/jwt.rs (1)
83-94: LGTM! Improved key ID calculation.The code now correctly uses the
jwt_key_idmethod to calculate the key ID from the public key, aligning with Kubernetes implementation.src/cluster_crypto/crypto_utils.rs (2)
28-46: LGTM! Correct implementation of JWT key ID calculation.The implementation correctly follows the Kubernetes approach:
- Derives public key from private key
- Encodes it in PKIX DER format
- Calculates SHA256 hash
- Base64URL encodes the hash
The code is well-documented with a reference to the original Kubernetes implementation.
305-401: LGTM! Comprehensive test coverage.The test module provides excellent validation by:
- Including a test key (safely obfuscated)
- Comparing against reference Go implementation
- Documenting the Go code used to generate the expected value
|
/retest |
|
/override ? |
|
@omertuc: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/override pull-ci-rh-ecosystem-edge-recert-main-e2e-aws-ovn-single-node-recert-serial |
|
/override pull-ci-rh-ecosystem-edge-recert-main-e2e-aws-ovn-single-node-recert-parallel |
|
@omertuc: Overrode contexts on behalf of omertuc: ci/prow/e2e-aws-ovn-single-node-recert-serial In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@omertuc: Overrode contexts on behalf of omertuc: ci/prow/e2e-aws-ovn-single-node-recert-parallel In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/cherry-pick release-4.18 |
|
@omertuc: new pull request created: #175 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@omertuc: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Background
Recert resigns JWTs found in the cluster. The kid field in the Kubernetes JWT header is a Base64URL encoded hash of the public key used to sign the JWT. The kid used by verifiers (e.g. kube-apiserver) to quickly identify the key used to sign the JWT, instead of brute-forcing.
Issue
See https://issues.redhat.com/browse/OCPBUGS-49972
The current implementation uses the SHA256 hash of the private key to calculate the kid. This is inconsistent with the Kubernetes implementation which uses the public key to calculate the kid.
In recent versions of Kubernetes, a kid mismatch leads to "unauthorized" errors, which eventually leads to a collapse of the cluster.
Solution
This commit changes the kid calculation to use the SHA256 hash of the public key instead of the private key, so that it becomes more consistent with Kubernetes
Backwards compatibility
Seeds processed with older versions of recert that don't contain this fix should still work if "rejuvenated" by newer versions of recert which do contain this fix. This is because the new version will simply overwrite the
kidfield with the correct value.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor