Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,7 @@ chrono = { default-features = false, version = "0.4.44" }
crates_io_api = { default-features = false, version = "0.12" }
clap = { default-features = false, version = "4" }
crc32c = { default-features = false, version = "0.6.8" }
der = { default-features = false, version = "0.7", features = ["std"] }
futures = { default-features = false, version = "0.3" }
h2 = { default-features = false, version = "0.4.13" }
hex = { default-features = false, version = "0.4.3" }
Expand Down
4 changes: 2 additions & 2 deletions src/auth/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ thiserror.workspace = true
time = { workspace = true, features = ["serde"] }
tokio = { workspace = true, features = ["fs", "process"] }
url.workspace = true
p256 = { workspace = true, features = ["ecdsa", "pem"], optional = true }
der = { workspace = true, optional = true }
jsonwebtoken = { workspace = true, optional = true }
# We do not use this directly, but without it the minimal-versions build breaks.
# See: https://github.com/Keats/jsonwebtoken/pull/481
Expand Down Expand Up @@ -81,7 +81,7 @@ default = ["default-idtoken-backend", "default-rustls-provider"]
# See the create top-level documentation for more information.
idtoken = ["dep:jsonwebtoken", "jsonwebtoken"]
# The `__gdch` feature enables support for GDCH service account credentials.
__gdch = ["dep:p256"]
__gdch = ["dep:der"]
# By default this crate enables the `aws_lc_rs` backend. Applications can
# link `google-cloud-auth` with `default-features = false, features = ["idtoken"]
# and then directly configure the `jsonwebtoken` features to select the
Expand Down
70 changes: 66 additions & 4 deletions src/auth/src/credentials/gdch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ use crate::token_cache::TokenCache;
use crate::{Result, errors};
use async_trait::async_trait;
use base64::prelude::{BASE64_URL_SAFE_NO_PAD, Engine as _};
use der::asn1::UintRef;
use der::{Reader, SliceReader};
use google_cloud_gax::backoff_policy::BackoffPolicyArg;
use google_cloud_gax::retry_policy::RetryPolicyArg;
use google_cloud_gax::retry_throttler::RetryThrottlerArg;
Expand Down Expand Up @@ -146,15 +148,63 @@ impl GdchServiceAccountTokenProvider {
let sig_der = signer
.sign(to_sign.as_bytes())
.map_err(errors::non_retryable)?;
let sig = p256::ecdsa::Signature::from_der(&sig_der).map_err(|e| {
errors::non_retryable_from_str(format!("failed to parse ecdsa DER signature: {}", e))
})?;
let encoded_sig = BASE64_URL_SAFE_NO_PAD.encode(&sig.to_bytes()[..]);
let sig_bytes = ecdsa_der_to_jose(&sig_der)?;
let encoded_sig = BASE64_URL_SAFE_NO_PAD.encode(&sig_bytes[..]);

Ok(format!("{}.{}", to_sign, encoded_sig))
}
}

/// Converts an ASN.1 DER-encoded ECDSA signature (`SEQUENCE { r INTEGER, s INTEGER }`)
/// into the fixed-size 64-byte raw concatenation (`r || s`) required by the JWS/JWT specification.
///
/// This implementation is zero-copy during parsing and avoids heap allocations entirely by writing
/// directly to a stack-allocated 64-byte array.
fn ecdsa_der_to_jose(der: &[u8]) -> Result<[u8; 64]> {
let mut reader = SliceReader::new(der).map_err(errors::non_retryable)?;
let (r_ref, s_ref) = reader
.sequence(|nested| {
let r = nested.decode::<UintRef<'_>>()?;
let s = nested.decode::<UintRef<'_>>()?;
Ok((r, s))
})
Comment on lines +165 to +170
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The closure passed to reader.sequence decodes two integers but does not verify that the sequence is then exhausted. To ensure strict adherence to the ASN.1 DER format for ECDSA signatures (SEQUENCE { r INTEGER, s INTEGER }), you should check that the nested reader is finished.

    let (r_ref, s_ref) = reader
        .sequence(|nested| {
            let r = nested.decode::<UintRef<'_>>()?;
            let s = nested.decode::<UintRef<'_>>()?;
            if !nested.is_finished() {
                return Err(nested.error(der::ErrorKind::TrailingData));
            }
            Ok((r, s))
        })
References
  1. Demand Explosive Correctness: Never swallow errors or ignore Result types. Fail loudly and explicitly when appropriate. (link)

.and_then(|res| reader.finish(res))
.map_err(errors::non_retryable)?;

let mut jose = [0u8; 64];
copy_jose_integer(&mut jose[..32], r_ref.as_bytes())?;
copy_jose_integer(&mut jose[32..], s_ref.as_bytes())?;
Ok(jose)
}

/// Copies a variable-length canonical ASN.1 DER integer into a fixed 32-byte big-endian buffer.
///
/// ASN.1 DER canonical unsigned integers enforce the minimum number of bytes. Critically:
/// - If the integer's most significant bit is set (`>= 0x80`), DER canonicalization prepends a `0x00`
/// byte to prevent negative interpretation in two's complement, resulting in a 33-byte slice. This
/// function securely strips that leading zero byte.
/// - If the integer is shorter than 32 bytes, it is prepended with leading zero padding to fill the
/// destination buffer precisely.
fn copy_jose_integer(dest: &mut [u8], bytes: &[u8]) -> Result<()> {
if bytes.len() > 33 {
return Err(errors::non_retryable_from_str(format!(
"invalid GDCH ECDSA signature integer length: {} bytes",
bytes.len()
)));
} else if bytes.len() == 33 {
if bytes[0] != 0 {
return Err(errors::non_retryable_from_str(
"invalid 33-byte GDCH ECDSA signature integer: leading byte is not zero",
));
}
dest.copy_from_slice(&bytes[1..]);
} else {
let offset = 32 - bytes.len();
dest[offset..].copy_from_slice(bytes);
}
Ok(())
}
Comment on lines +188 to +206
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The function copy_jose_integer uses hardcoded values 32 and 33. It is more robust and idiomatic to use dest.len() and dest.len() + 1 instead. Additionally, the function relies on the caller to zero-initialize the destination buffer for correct padding. It is safer to explicitly zero the padding area within the function.

fn copy_jose_integer(dest: &mut [u8], bytes: &[u8]) -> Result<()> {
    let dest_len = dest.len();
    if bytes.len() > dest_len + 1 {
        return Err(errors::non_retryable_from_str(format!(
            "invalid GDCH ECDSA signature integer length: {} bytes",
            bytes.len()
        )));
    } else if bytes.len() == dest_len + 1 {
        if bytes[0] != 0 {
            return Err(errors::non_retryable_from_str(
                format!("invalid {}-byte GDCH ECDSA signature integer: leading byte is not zero", bytes.len()),
            ));
        }
        dest.copy_from_slice(&bytes[1..]);
    } else {
        let offset = dest_len - bytes.len();
        dest[..offset].fill(0);
        dest[offset..].copy_from_slice(bytes);
    }
    Ok(())
}
References
  1. Reject 'Code Poetry': Dismantle complex abstractions used for simple tasks. Prefer simplicity over cleverness. (link)


#[async_trait]
impl TokenProvider for GdchServiceAccountTokenProvider {
async fn token(&self) -> Result<Token> {
Expand Down Expand Up @@ -324,6 +374,7 @@ impl AccessTokenCredentialsProvider for GdchServiceAccountCredentials {
mod tests {
use super::*;
use httptest::{Expectation, Server, matchers::*, responders::*};
use p256::ecdsa::signature::Verifier;
use serde_json::json;

type TestResult = anyhow::Result<()>;
Expand Down Expand Up @@ -443,6 +494,17 @@ mod tests {
"system:serviceaccount:test-project:test-name"
);
assert_eq!(claims_json["aud"], "http://localhost/token");

let public_key = crate::credentials::tests::ES256_PRIVATE_KEY.public_key();
let verifying_key = p256::ecdsa::VerifyingKey::from(&public_key);
let sig_bytes = BASE64_URL_SAFE_NO_PAD.decode(parts[2])?;
let signature = p256::ecdsa::Signature::from_slice(&sig_bytes)
.map_err(|e| anyhow::anyhow!("invalid signature: {:?}", e))?;
let signed_data = format!("{}.{}", parts[0], parts[1]);
verifying_key
.verify(signed_data.as_bytes(), &signature)
.map_err(|e| anyhow::anyhow!("verification failed: {:?}", e))?;

Ok(())
}

Expand Down
Loading