Prepare crates for publishing to crates.io#23
Conversation
gregnazario
commented
Feb 5, 2026
- Update release workflow to publish both crates in correct order (macros first, then SDK after crates.io indexing)
- Add version requirement for aptos-sdk-macros dependency
- Fix README version references (0.1 -> 0.3)
There was a problem hiding this comment.
Pull request overview
This PR prepares both Aptos SDK crates for publication to crates.io by updating the release workflow to handle dependencies correctly and fixing documentation version references.
Changes:
- Updated release workflow to publish
aptos-sdk-macrosfirst, thenaptos-sdkafter waiting for crates.io indexing - Added version requirement for
aptos-sdk-macrosdependency in workspaceCargo.toml - Fixed README version reference from 0.1 to 0.3
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| .github/workflows/release.yml | Added separate publish steps for macros crate with indexing wait time between publications |
| Cargo.toml | Added version constraint for aptos-sdk-macros workspace dependency |
| crates/aptos-sdk/README.md | Updated version reference in installation example |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
708d5aa to
41d70a5
Compare
- Update release workflow to publish both crates in correct order (macros first, then SDK after crates.io indexing) - Add version requirement for aptos-sdk-macros dependency - Fix README version references (0.1 -> 0.3)
CI checkout from a tag should always be clean. If there are dirty files during release, that's a problem we want to catch.
Ensures builds use exactly what's in Cargo.lock and fails if the lockfile is out of sync with Cargo.toml.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Update time crate 0.3.44 -> 0.3.47 - Update jsonwebtoken 9.3 -> 10.3 with aws_lc_rs backend - Use dangerous_insecure_decode API (v10 migration) - aws_lc_rs avoids RSA timing vulnerability in rust_crypto backend All security audits pass with no vulnerabilities.
Replace insecure JWT decoding with proper JWKS-based verification: - Add fetch_jwks() to retrieve keys from OIDC provider endpoints - Add decode_and_verify_jwt() for full signature verification - Update from_jwt() and refresh_proof() to verify JWT signatures - Add from_verified_claims() helper for unit testing - Add comprehensive tests for the new verification flow This improves security by verifying JWT signatures directly in the SDK using the OIDC provider's JWKS endpoint, rather than relying solely on external services for validation.
41d70a5 to
4667533
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 17 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -359,7 +361,20 @@ impl KeylessAccount { | |||
| pepper_service: &dyn PepperService, | |||
| prover_service: &dyn ProverService, | |||
| ) -> AptosResult<Self> { | |||
| let claims = decode_claims(jwt)?; | |||
| // First, decode without verification to get the issuer for JWKS lookup | |||
| let unverified_claims = decode_claims_unverified(jwt)?; | |||
| let issuer = unverified_claims | |||
| .iss | |||
| .as_ref() | |||
| .ok_or_else(|| AptosError::InvalidJwt("missing iss claim".into()))?; | |||
|
|
|||
| // Determine provider and fetch JWKS | |||
| let provider = OidcProvider::from_issuer(issuer); | |||
| let client = reqwest::Client::new(); | |||
| let jwks = fetch_jwks(&client, provider.jwks_url()).await?; | |||
|
|
|||
| // Now verify and decode the JWT properly | |||
| let claims = decode_and_verify_jwt(jwt, &jwks)?; | |||
There was a problem hiding this comment.
The from_jwt method now performs network requests (fetching JWKS) which is a breaking API change in behavior. Previously, this method only did local JWT decoding, but now it makes HTTP calls that can fail due to network issues, timeouts, or rate limiting. This significantly changes the failure modes and performance characteristics of the method.
Consider:
- Documenting this breaking change clearly in release notes
- Adding a network-related error to the function documentation
- Potentially adding an alternative method that accepts pre-fetched JWKS to give users more control over network calls and caching
| // Determine the algorithm from the JWK or header | ||
| let algorithm = signing_key | ||
| .common | ||
| .key_algorithm | ||
| .and_then(|alg| match alg { | ||
| jsonwebtoken::jwk::KeyAlgorithm::RS256 => Some(Algorithm::RS256), | ||
| jsonwebtoken::jwk::KeyAlgorithm::RS384 => Some(Algorithm::RS384), | ||
| jsonwebtoken::jwk::KeyAlgorithm::RS512 => Some(Algorithm::RS512), | ||
| jsonwebtoken::jwk::KeyAlgorithm::ES256 => Some(Algorithm::ES256), | ||
| jsonwebtoken::jwk::KeyAlgorithm::ES384 => Some(Algorithm::ES384), | ||
| _ => None, | ||
| }) | ||
| .unwrap_or(header.alg); | ||
|
|
There was a problem hiding this comment.
The algorithm matching only supports a limited set of algorithms (RS256, RS384, RS512, ES256, ES384) but falls back to the header algorithm for any other algorithm. This fallback could be a security risk if the JWKS contains an algorithm the code doesn't explicitly handle, as it trusts the JWT header's algorithm claim.
Consider either:
- Explicitly rejecting unsupported algorithms instead of falling back to the header algorithm
- Verifying that the fallback algorithm matches what's expected for the key type in the JWKS
This prevents potential algorithm substitution attacks where an attacker might try to use a weaker algorithm.
| // Determine the algorithm from the JWK or header | |
| let algorithm = signing_key | |
| .common | |
| .key_algorithm | |
| .and_then(|alg| match alg { | |
| jsonwebtoken::jwk::KeyAlgorithm::RS256 => Some(Algorithm::RS256), | |
| jsonwebtoken::jwk::KeyAlgorithm::RS384 => Some(Algorithm::RS384), | |
| jsonwebtoken::jwk::KeyAlgorithm::RS512 => Some(Algorithm::RS512), | |
| jsonwebtoken::jwk::KeyAlgorithm::ES256 => Some(Algorithm::ES256), | |
| jsonwebtoken::jwk::KeyAlgorithm::ES384 => Some(Algorithm::ES384), | |
| _ => None, | |
| }) | |
| .unwrap_or(header.alg); | |
| // Determine the algorithm strictly from the JWK and ensure it matches the header | |
| let jwk_alg = signing_key | |
| .common | |
| .key_algorithm | |
| .ok_or_else(|| { | |
| AptosError::InvalidJwt("JWK missing 'alg' (key_algorithm) field".into()) | |
| })?; | |
| let algorithm = match jwk_alg { | |
| jsonwebtoken::jwk::KeyAlgorithm::RS256 => Algorithm::RS256, | |
| jsonwebtoken::jwk::KeyAlgorithm::RS384 => Algorithm::RS384, | |
| jsonwebtoken::jwk::KeyAlgorithm::RS512 => Algorithm::RS512, | |
| jsonwebtoken::jwk::KeyAlgorithm::ES256 => Algorithm::ES256, | |
| jsonwebtoken::jwk::KeyAlgorithm::ES384 => Algorithm::ES384, | |
| _ => { | |
| return Err(AptosError::InvalidJwt(format!( | |
| "unsupported JWK algorithm: {jwk_alg:?}" | |
| ))); | |
| } | |
| }; | |
| // Ensure the JWT header algorithm matches the JWK algorithm to prevent substitution | |
| if header.alg != algorithm { | |
| return Err(AptosError::InvalidJwt(format!( | |
| "JWT header algorithm ({:?}) does not match JWK algorithm ({:?})", | |
| header.alg, algorithm | |
| ))); | |
| } |
.github/workflows/release.yml
Outdated
| - name: Wait for crates.io indexing | ||
| if: github.event.inputs.dry_run != 'true' || github.event_name == 'push' | ||
| run: sleep 90 |
There was a problem hiding this comment.
The 90-second wait for crates.io indexing may not be sufficient in all cases. While crates.io typically indexes within this timeframe, there can be delays during high load or maintenance periods. If the wait is insufficient, the subsequent cargo publish -p aptos-sdk will fail because the versioned dependency on aptos-sdk-macros won't be available yet.
Consider:
- Increasing the wait time to 120-180 seconds for more reliability
- Implementing a retry mechanism that checks if the crate is available before publishing
- Using a polling mechanism with the crates.io API to verify the crate is indexed before proceeding
The current approach risks CI failures that require manual re-runs.
| /// This is used only to extract the issuer before we know which JWKS to fetch. | ||
| /// The JWT is fully verified afterwards using `decode_and_verify_jwt`. | ||
| fn decode_claims_unverified(jwt: &str) -> AptosResult<JwtClaims> { | ||
| // Use dangerous decode only for initial issuer extraction | ||
| // The JWT will be properly verified after we fetch the JWKS |
There was a problem hiding this comment.
While the use of dangerous::insecure_decode is appropriate here (to extract the issuer before fetching JWKS), the comment should be more explicit about why this is safe. The current comment explains the purpose but could be clearer about the security implications.
Consider adding a note that this is safe because:
- The extracted issuer is only used to determine which JWKS endpoint to fetch from
- The JWT is fully verified immediately after in
decode_and_verify_jwt - No security decisions are made based on the unverified claims
This helps future maintainers understand the security model more clearly.
| /// This is used only to extract the issuer before we know which JWKS to fetch. | |
| /// The JWT is fully verified afterwards using `decode_and_verify_jwt`. | |
| fn decode_claims_unverified(jwt: &str) -> AptosResult<JwtClaims> { | |
| // Use dangerous decode only for initial issuer extraction | |
| // The JWT will be properly verified after we fetch the JWKS | |
| /// This is used only to extract the issuer (and other metadata) before we know | |
| /// which JWKS endpoint to fetch. This is safe because: | |
| /// 1. The extracted issuer is only used to determine which JWKS endpoint to fetch. | |
| /// 2. The JWT is fully verified immediately afterwards using `decode_and_verify_jwt`. | |
| /// 3. No security decisions are made based on these unverified claims. | |
| fn decode_claims_unverified(jwt: &str) -> AptosResult<JwtClaims> { | |
| // Use dangerous decode only for initial issuer extraction to select the JWKS. | |
| // The JWT is not trusted at this point: no authorization decisions are made | |
| // based on these unverified claims, and the token is fully verified (including | |
| // signature and claims validation) in `decode_and_verify_jwt` right after the | |
| // appropriate JWKS has been fetched. |
| /// - The JWT cannot be decoded or is missing required claims (iss, aud, sub, nonce) | ||
| /// - The JWT nonce doesn't match the ephemeral key's nonce | ||
| /// - The JWT is expired | ||
| /// - The JWKS cannot be fetched from the provider |
There was a problem hiding this comment.
The documentation states "The JWKS cannot be fetched from the provider" but doesn't mention potential network-related failures such as timeouts, DNS resolution failures, or connection errors. Consider expanding this error documentation to be more comprehensive about all the ways JWKS fetching can fail, which helps users understand what errors to handle.
| /// - The JWKS cannot be fetched from the provider | |
| /// - The JWKS cannot be fetched from the provider (e.g., due to network timeouts, DNS resolution failures, TLS/connection errors, unexpected HTTP status codes, or invalid/malformed JWKS responses) |
.github/workflows/release.yml
Outdated
|
|
||
| # Publish macros crate first (aptos-sdk depends on it) | ||
| - name: Publish aptos-sdk-macros (dry run) | ||
| if: github.event.inputs.dry_run == 'true' |
There was a problem hiding this comment.
The condition logic for the dry-run publish steps is incorrect. When dry_run is 'true', the dry-run step should execute, but the condition github.event.inputs.dry_run == 'true' will not work correctly for boolean inputs.
Since the workflow defines dry_run as type: boolean, it will be a boolean value (true/false), not a string ('true'/'false'). The condition should be:
if: github.event.inputs.dry_run == trueorif: inputs.dry_run == true
Additionally, the non-dry-run conditions have github.event.inputs.dry_run != 'true' || github.event_name == 'push' which is also problematic for the same reason.
.github/workflows/release.yml
Outdated
|
|
||
| # Publish main SDK crate | ||
| - name: Publish aptos-sdk (dry run) | ||
| if: github.event.inputs.dry_run == 'true' |
There was a problem hiding this comment.
The condition has the same boolean vs string comparison issue as the other steps. Since dry_run is defined as type: boolean in the workflow inputs, this should use boolean comparison instead of string comparison.
.github/workflows/release.yml
Outdated
|
|
||
| # Wait for crates.io to index the macros crate | ||
| - name: Wait for crates.io indexing | ||
| if: github.event.inputs.dry_run != 'true' || github.event_name == 'push' |
There was a problem hiding this comment.
The condition has the same boolean vs string comparison issue. Since dry_run is defined as type: boolean, this should use boolean comparison instead of string comparison.
.github/workflows/release.yml
Outdated
| run: cargo publish -p aptos-sdk-macros --locked --dry-run | ||
|
|
||
| - name: Publish aptos-sdk-macros | ||
| if: github.event.inputs.dry_run != 'true' || github.event_name == 'push' |
There was a problem hiding this comment.
The condition logic is incorrect. When dry_run is a boolean input type (as defined in the workflow), comparing it to the string 'true' will not work as expected. The condition should use boolean comparison instead of string comparison:
if: inputs.dry_run != true && github.event_name == 'push'orif: (github.event.inputs.dry_run != true || github.event_name == 'push')
The current condition github.event.inputs.dry_run != 'true' || github.event_name == 'push' will always evaluate to true when the input is a boolean, potentially causing unintended publishes during dry-run mode.
| async fn fetch_jwks(client: &reqwest::Client, jwks_url: &str) -> AptosResult<JwkSet> { | ||
| let response = client.get(jwks_url).send().await?; | ||
|
|
||
| if !response.status().is_success() { | ||
| return Err(AptosError::InvalidJwt(format!( | ||
| "JWKS endpoint returned status: {}", | ||
| response.status() | ||
| ))); | ||
| } | ||
|
|
||
| let jwks: JwkSet = response.json().await?; | ||
| Ok(jwks) | ||
| } |
There was a problem hiding this comment.
The new fetch_jwks function lacks test coverage. Since this function performs critical security operations (fetching and parsing JWKS), it should have dedicated tests, potentially using a mock HTTP server or testing framework. Consider adding tests that verify:
- Successful JWKS fetching and parsing
- Handling of HTTP error responses (4xx, 5xx)
- Handling of malformed JSON responses
- Network timeout scenarios
release.yml: - Fix boolean comparison for dry_run input (use `== true` not `== 'true'`) - Increase crates.io indexing wait time to 120s for reliability keyless.rs: - Fix algorithm substitution vulnerability: strictly require JWK algorithm field and verify JWT header algorithm matches - Add 10-second timeout to JWKS fetch requests - Remove sensitive key ID from error messages - Improve documentation for decode_claims_unverified with security rationale - Expand error documentation for JWKS fetching failures - Add from_jwt_with_jwks and refresh_proof_with_jwks methods for JWKS caching - Document network request behavior in from_jwt and refresh_proof - Re-export JwkSet type for use with the new methods
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let algorithm = match jwk_alg { | ||
| jsonwebtoken::jwk::KeyAlgorithm::RS256 => Algorithm::RS256, | ||
| jsonwebtoken::jwk::KeyAlgorithm::RS384 => Algorithm::RS384, | ||
| jsonwebtoken::jwk::KeyAlgorithm::RS512 => Algorithm::RS512, | ||
| jsonwebtoken::jwk::KeyAlgorithm::ES256 => Algorithm::ES256, | ||
| jsonwebtoken::jwk::KeyAlgorithm::ES384 => Algorithm::ES384, | ||
| _ => { | ||
| return Err(AptosError::InvalidJwt(format!( | ||
| "unsupported JWK algorithm: {jwk_alg:?}" | ||
| ))); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The algorithm matching logic doesn't support some valid OIDC algorithms. Consider adding support for:
- ES512 (ECDSA with P-521 curve)
- PS256, PS384, PS512 (RSA-PSS algorithms)
- EdDSA
While most OIDC providers use RS256, some providers may use these other algorithms. Without support, JWTs from those providers will be rejected with an "unsupported JWK algorithm" error, even though they are valid and secure algorithms supported by the jsonwebtoken crate.
| fn decode_and_verify_jwt(jwt: &str, jwks: &JwkSet) -> AptosResult<JwtClaims> { | ||
| // Decode header to get the key ID | ||
| let header = decode_header(jwt) | ||
| .map_err(|e| AptosError::InvalidJwt(format!("failed to decode JWT header: {e}")))?; | ||
|
|
||
| let kid = header | ||
| .kid | ||
| .as_ref() | ||
| .ok_or_else(|| AptosError::InvalidJwt("JWT header missing 'kid' field".into()))?; | ||
|
|
||
| // Find the matching key in the JWKS | ||
| let signing_key = jwks.find(kid).ok_or_else(|| { | ||
| AptosError::InvalidJwt("no matching key found for provided key identifier".into()) | ||
| })?; | ||
|
|
||
| // Create decoding key from JWK | ||
| let decoding_key = DecodingKey::from_jwk(signing_key) | ||
| .map_err(|e| AptosError::InvalidJwt(format!("failed to create decoding key: {e}")))?; | ||
|
|
||
| // Determine the algorithm strictly from the JWK to prevent algorithm substitution attacks | ||
| let jwk_alg = signing_key | ||
| .common | ||
| .key_algorithm | ||
| .ok_or_else(|| AptosError::InvalidJwt("JWK missing 'alg' (key_algorithm) field".into()))?; | ||
|
|
||
| let algorithm = match jwk_alg { | ||
| jsonwebtoken::jwk::KeyAlgorithm::RS256 => Algorithm::RS256, | ||
| jsonwebtoken::jwk::KeyAlgorithm::RS384 => Algorithm::RS384, | ||
| jsonwebtoken::jwk::KeyAlgorithm::RS512 => Algorithm::RS512, | ||
| jsonwebtoken::jwk::KeyAlgorithm::ES256 => Algorithm::ES256, | ||
| jsonwebtoken::jwk::KeyAlgorithm::ES384 => Algorithm::ES384, | ||
| _ => { | ||
| return Err(AptosError::InvalidJwt(format!( | ||
| "unsupported JWK algorithm: {jwk_alg:?}" | ||
| ))); | ||
| } | ||
| }; | ||
|
|
||
| // Ensure the JWT header algorithm matches the JWK algorithm to prevent substitution | ||
| if header.alg != algorithm { | ||
| return Err(AptosError::InvalidJwt(format!( | ||
| "JWT header algorithm ({:?}) does not match JWK algorithm ({:?})", | ||
| header.alg, algorithm | ||
| ))); | ||
| } | ||
|
|
||
| // Configure validation - we'll validate exp ourselves with more detailed errors | ||
| let mut validation = Validation::new(algorithm); | ||
| validation.validate_exp = false; | ||
| validation.validate_aud = false; // We'll check aud after decoding | ||
| validation.set_required_spec_claims::<String>(&[]); | ||
|
|
||
| let data = decode::<JwtClaims>(jwt, &DecodingKey::from_secret(&[]), &validation) | ||
| let data = decode::<JwtClaims>(jwt, &decoding_key, &validation) | ||
| .map_err(|e| AptosError::InvalidJwt(format!("JWT verification failed: {e}")))?; | ||
|
|
||
| Ok(data.claims) | ||
| } |
There was a problem hiding this comment.
The JWT signature verification implementation lacks test coverage. While unit tests exist for from_verified_claims (which bypasses verification), there are no tests for:
- decode_and_verify_jwt function
- from_jwt_with_jwks method
- refresh_proof_with_jwks method
- Algorithm matching logic (lines 777-788)
- JWT header algorithm vs JWK algorithm mismatch detection (lines 791-796)
Consider adding integration tests or mocked tests that verify JWT signature validation works correctly, invalid signatures are rejected, and algorithm substitution attacks are prevented.
| async fn fetch_jwks(client: &reqwest::Client, jwks_url: &str) -> AptosResult<JwkSet> { | ||
| let response = client | ||
| .get(jwks_url) | ||
| .timeout(JWKS_FETCH_TIMEOUT) |
There was a problem hiding this comment.
The timeout is configured twice: once when building the reqwest Client (line 388) and again on the individual request (line 723). The request-level timeout is redundant since the client already has a timeout configured.
Remove the .timeout() call on line 723, as the client-level timeout at line 388 already applies to all requests made by that client.
| .timeout(JWKS_FETCH_TIMEOUT) |
- Add support for more OIDC algorithms: PS256, PS384, PS512, EdDSA - Remove redundant timeout on fetch_jwks request (client already has timeout) - Add test coverage for decode_and_verify_jwt error paths: - Missing kid in JWT header - No matching key in JWKS - Invalid JWT format
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async fn fetch_jwks(client: &reqwest::Client, jwks_url: &str) -> AptosResult<JwkSet> { | ||
| // Note: timeout is configured on the client, not per-request | ||
| let response = client.get(jwks_url).send().await?; | ||
|
|
||
| if !response.status().is_success() { | ||
| return Err(AptosError::InvalidJwt(format!( | ||
| "JWKS endpoint returned status: {}", | ||
| response.status() | ||
| ))); | ||
| } | ||
|
|
||
| let jwks: JwkSet = response.json().await?; | ||
| Ok(jwks) | ||
| } |
There was a problem hiding this comment.
The fetch_jwks function lacks test coverage. There are no tests verifying that it correctly handles HTTP errors, non-success status codes, network timeouts, or invalid JSON responses. Consider adding tests to ensure proper error handling for these scenarios, especially since this function makes external network calls that are critical for security.
.github/workflows/release.yml
Outdated
| - name: Publish aptos-sdk | ||
| if: github.event.inputs.dry_run != 'true' || github.event_name == 'push' | ||
| run: cargo publish -p aptos-sdk | ||
| if: inputs.dry_run != true || github.event_name == 'push' |
There was a problem hiding this comment.
The condition logic for publishing the SDK crate appears problematic. When triggered via workflow_dispatch with dry_run=false, the condition inputs.dry_run != true || github.event_name == 'push' will be true (because inputs.dry_run is false), so it will attempt to publish. However, when triggered via tag push, inputs.dry_run will be undefined/null in the push event context. In GitHub Actions, comparing undefined values with booleans can have unexpected behavior. Consider using fromJSON(inputs.dry_run || 'false') or checking github.event_name == 'push' first to ensure the condition works correctly in both trigger scenarios.
| pub async fn from_jwt_with_jwks( | ||
| jwt: &str, | ||
| jwks: &JwkSet, | ||
| ephemeral_key: EphemeralKeyPair, | ||
| pepper_service: &dyn PepperService, | ||
| prover_service: &dyn ProverService, | ||
| ) -> AptosResult<Self> { |
There was a problem hiding this comment.
The new from_jwt_with_jwks method lacks test coverage. While from_jwt is indirectly tested through from_verified_claims, there are no tests that verify the JWT signature validation logic in from_jwt_with_jwks works correctly with actual JWKS data. Consider adding tests that validate the method correctly handles valid JWTs with matching JWKS, and properly rejects JWTs with invalid signatures or mismatched keys.
| pub async fn refresh_proof_with_jwks( | ||
| &mut self, | ||
| jwt: &str, | ||
| jwks: &JwkSet, | ||
| prover_service: &dyn ProverService, | ||
| ) -> AptosResult<()> { |
There was a problem hiding this comment.
The new refresh_proof_with_jwks method lacks test coverage. There are no tests verifying that this method correctly validates JWT signatures against provided JWKS, properly handles nonce mismatches, or validates that the JWT identity matches the account. Consider adding tests to ensure the JWT verification and validation logic works correctly.
Make conditions explicit about event type to avoid undefined value comparisons when inputs.dry_run is not set during tag push events: - Dry-run: only for workflow_dispatch with dry_run=true - Actual publish: for tag push OR workflow_dispatch with dry_run=false