feat(trust): Support GitLab ID tokens for attestation#169
feat(trust): Support GitLab ID tokens for attestation#169erran wants to merge 1 commit intoalways-further:mainfrom
Conversation
Summary of ChangesHello @erran, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces comprehensive support for GitLab CI/CD environments within the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds valuable support for GitLab CI/CD environments for keyless signing and verification, which is a great enhancement. My review focuses on improving the robustness and maintainability of this new functionality. I've identified a critical issue where missing environment variables could lead to silently generating incorrect attestations, a high-severity race condition in the new tests, and a medium-severity code duplication that should be refactored. Addressing these points will make the GitLab integration more reliable and secure.
| fn gitlab_keyless_predicate() -> Option<serde_json::Value> { | ||
| if std::env::var("GITLAB_CI").as_deref() != Ok("true") { | ||
| return None; | ||
| } | ||
|
|
||
| let host = std::env::var("CI_SERVER_HOST").unwrap_or_else(|_| "gitlab.com".to_string()); | ||
| let port = std::env::var("CI_SERVER_PORT").unwrap_or_else(|_| "443".to_string()); | ||
| let project_path = std::env::var("CI_PROJECT_PATH").unwrap_or_default(); | ||
|
|
||
| let host_authority = if port == "443" { | ||
| host.clone() | ||
| } else { | ||
| format!("{host}:{port}") | ||
| }; | ||
|
|
||
| let git_ref = match std::env::var("CI_COMMIT_TAG") { | ||
| Ok(tag) if !tag.is_empty() => format!("refs/tags/{tag}"), | ||
| _ => format!( | ||
| "refs/heads/{}", | ||
| std::env::var("CI_COMMIT_REF_NAME").unwrap_or_default() | ||
| ), | ||
| }; | ||
|
|
||
| let workflow = format!("{host_authority}/{project_path}//.gitlab-ci.yml@{git_ref}"); | ||
|
|
||
| Some(serde_json::json!({ | ||
| "version": 1, | ||
| "signer": { | ||
| "kind": "keyless", | ||
| "oidc_issuer": std::env::var("CI_SERVER_URL") | ||
| .unwrap_or_else(|_| "https://gitlab.com".to_string()), | ||
| "repository": project_path, | ||
| "workflow": workflow, | ||
| "ref": git_ref | ||
| } | ||
| })) | ||
| } |
There was a problem hiding this comment.
Using unwrap_or_else and unwrap_or_default for required GitLab CI environment variables can lead to silently generating incorrect attestations if those variables are not present. For example, if CI_PROJECT_PATH is missing, it defaults to an empty string, resulting in a malformed workflow URL. This is particularly risky for self-hosted GitLab instances where the defaults for gitlab.com would be incorrect.
To make this more robust, this function should return a Result and fail explicitly if a required environment variable is missing. This ensures that you either generate a correct predicate or fail fast, preventing bad signatures.
This will require updating build_keyless_predicate to return a Result<serde_json::Value> and its callers (sign_file_keyless and run_sign_multi_keyless) to handle the result using ?.
fn gitlab_keyless_predicate() -> Result<Option<serde_json::Value>> {
if std::env::var("GITLAB_CI").as_deref() != Ok("true") {
return Ok(None);
}
fn get_env(key: &str) -> Result<String> {
std::env::var(key).map_err(|_| nono::NonoError::TrustSigning {
path: String::new(),
reason: format!("missing required GitLab CI environment variable: {key}"),
})
}
let host = get_env("CI_SERVER_HOST")?;
let port = get_env("CI_SERVER_PORT")?;
let project_path = get_env("CI_PROJECT_PATH")?;
let oidc_issuer = get_env("CI_SERVER_URL")?;
let host_authority = if port == "443" {
host
} else {
format!("{host}:{port}")
};
let git_ref = match std::env::var("CI_COMMIT_TAG") {
Ok(tag) if !tag.is_empty() => format!("refs/tags/{tag}"),
_ => format!(
"refs/heads/{}",
get_env("CI_COMMIT_REF_NAME")?
),
};
let workflow = format!("{host_authority}/{project_path}//.gitlab-ci.yml@{git_ref}");
Ok(Some(serde_json::json!({
"version": 1,
"signer": {
"kind": "keyless",
"oidc_issuer": oidc_issuer,
"repository": project_path,
"workflow": workflow,
"ref": git_ref
}
})))
}| fn build_keyless_predicate_defaults_to_github() { | ||
| std::env::remove_var("GITLAB_CI"); | ||
| let predicate = build_keyless_predicate(); | ||
| let signer = &predicate["signer"]; | ||
| assert_eq!(signer["kind"], "keyless"); | ||
| assert!(signer.get("oidc_issuer").is_some()); | ||
| assert!(signer.get("repository").is_some()); | ||
| assert!(signer.get("workflow").is_some()); | ||
| assert!(signer.get("ref").is_some()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn build_keyless_predicate_gitlab() { | ||
| std::env::set_var("GITLAB_CI", "true"); | ||
| std::env::set_var("CI_SERVER_HOST", "gitlab.com"); | ||
| std::env::set_var("CI_SERVER_PORT", "443"); | ||
| std::env::set_var("CI_PROJECT_PATH", "my-group/my-project"); | ||
| std::env::set_var("CI_COMMIT_REF_NAME", "main"); | ||
| std::env::remove_var("CI_COMMIT_TAG"); | ||
| let predicate = build_keyless_predicate(); | ||
| let signer = &predicate["signer"]; | ||
| assert_eq!(signer["kind"], "keyless"); | ||
| assert_eq!(signer["repository"], "my-group/my-project"); | ||
| assert_eq!( | ||
| signer["workflow"], | ||
| "gitlab.com/my-group/my-project//.gitlab-ci.yml@refs/heads/main" | ||
| ); | ||
| assert_eq!(signer["ref"], "refs/heads/main"); | ||
| std::env::remove_var("GITLAB_CI"); | ||
| std::env::remove_var("CI_SERVER_HOST"); | ||
| std::env::remove_var("CI_SERVER_PORT"); | ||
| std::env::remove_var("CI_PROJECT_PATH"); | ||
| std::env::remove_var("CI_COMMIT_REF_NAME"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn build_keyless_predicate_gitlab_custom_port() { | ||
| std::env::set_var("GITLAB_CI", "true"); | ||
| std::env::set_var("CI_SERVER_HOST", "gitlab.example.com"); | ||
| std::env::set_var("CI_SERVER_PORT", "8443"); | ||
| std::env::set_var("CI_PROJECT_PATH", "team/app"); | ||
| std::env::set_var("CI_COMMIT_REF_NAME", "develop"); | ||
| std::env::remove_var("CI_COMMIT_TAG"); | ||
| let predicate = build_keyless_predicate(); | ||
| let signer = &predicate["signer"]; | ||
| assert_eq!( | ||
| signer["workflow"], | ||
| "gitlab.example.com:8443/team/app//.gitlab-ci.yml@refs/heads/develop" | ||
| ); | ||
| std::env::remove_var("GITLAB_CI"); | ||
| std::env::remove_var("CI_SERVER_HOST"); | ||
| std::env::remove_var("CI_SERVER_PORT"); | ||
| std::env::remove_var("CI_PROJECT_PATH"); | ||
| std::env::remove_var("CI_COMMIT_REF_NAME"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn build_keyless_predicate_gitlab_takes_precedence() { | ||
| std::env::set_var("GITLAB_CI", "true"); | ||
| std::env::set_var("CI_SERVER_URL", "https://gitlab.example.com"); | ||
| let predicate = build_keyless_predicate(); | ||
| let signer = &predicate["signer"]; | ||
| assert_eq!(signer["oidc_issuer"], "https://gitlab.example.com"); | ||
| std::env::remove_var("GITLAB_CI"); | ||
| std::env::remove_var("CI_SERVER_URL"); | ||
| } |
There was a problem hiding this comment.
Tests that modify environment variables (std::env::set_var, std::env::remove_var) are modifying a global, shared resource. Since Rust tests run in parallel by default, this can cause race conditions between tests, leading to flaky and non-deterministic failures that are hard to debug.
To fix this, you should ensure these tests are serialized by using a std::sync::Mutex. You can create a static mutex and lock it at the beginning of each test that modifies the environment.
Here's a helper function you can add to your tests module:
use std::sync::{Mutex, OnceLock};
fn env_mutex() -> &'static Mutex<()> {
static MUTEX: OnceLock<Mutex<()>> = OnceLock::new();
MUTEX.get_or_init(|| Mutex::new(()))
}Then, at the start of each test that modifies environment variables, add let _guard = env_mutex().lock().unwrap();. This should be applied to build_keyless_predicate_defaults_to_github, build_keyless_predicate_gitlab, build_keyless_predicate_gitlab_custom_port, and build_keyless_predicate_gitlab_takes_precedence.
|
awesome @erran , I had planned to do this , so grateful you're taking this on. Let me know of any thoughts you have on the current setup. I largely based it off how sigstore-python works. |
2 participants
What does this PR change?
Adds support for:
Why?
Currently GitHub is the assumed publisher despite being able to lookup ambient credentials for keyless signing. It’d be a great to leverage nono for agent instruction attestation in GitLab managed environments too.