Conversation
Summary of ChangesHello @avidal, 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 a significant security enhancement by integrating OIDC push authorization into 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 introduces OIDC push authorization, a significant and well-implemented feature. The code is clean, well-structured, and includes comprehensive documentation and tests, which is excellent. The use of the Option pattern for server configuration and the creation of a dedicated oidc package are great design choices. I've found one potential bug in the glob matching logic that could lead to an infinite loop, for which I've provided a suggestion.
| func MatchGlob(pattern, value string) bool { | ||
| // Fast path: no wildcards means exact match. | ||
| if !strings.Contains(pattern, "*") { | ||
| return pattern == value | ||
| } | ||
|
|
||
| parts := strings.Split(pattern, "*") | ||
|
|
||
| // Check that the value starts with the first segment and ends with the last. | ||
| if !strings.HasPrefix(value, parts[0]) { | ||
| return false | ||
| } | ||
|
|
||
| if !strings.HasSuffix(value, parts[len(parts)-1]) { | ||
| return false | ||
| } | ||
|
|
||
| // Walk through remaining segments ensuring they appear in order. | ||
| remaining := value[len(parts[0]):] | ||
|
|
||
| for _, part := range parts[1:] { | ||
| idx := strings.Index(remaining, part) | ||
| if idx < 0 { | ||
| return false | ||
| } | ||
|
|
||
| remaining = remaining[idx+len(part):] | ||
| } | ||
|
|
||
| return true | ||
| } |
There was a problem hiding this comment.
The MatchGlob function has a bug where consecutive * characters (e.g., a**b) in the pattern can cause an infinite loop. This happens because strings.Split produces empty strings for consecutive delimiters, and the subsequent loop does not advance when processing an empty part, leading to an infinite loop.
A simple way to fix this is to normalize the pattern by replacing any occurrences of ** with a single * before splitting, as they are functionally equivalent for this glob implementation.
func MatchGlob(pattern, value string) bool {
// Fast path: no wildcards means exact match.
if !strings.Contains(pattern, "*") {
return pattern == value
}
// Normalize multiple wildcards to a single one, as they are equivalent.
// This also prevents an infinite loop in the logic below when Split produces empty parts.
for strings.Contains(pattern, "**") {
pattern = strings.ReplaceAll(pattern, "**", "*")
}
parts := strings.Split(pattern, "*")
// Check that the value starts with the first segment and ends with the last.
if !strings.HasPrefix(value, parts[0]) {
return false
}
if !strings.HasSuffix(value, parts[len(parts)-1]) {
return false
}
// Walk through remaining segments ensuring they appear in order.
remaining := value[len(parts[0]):]
for _, part := range parts[1:] {
idx := strings.Index(remaining, part)
if idx < 0 {
return false
}
remaining = remaining[idx+len(part):]
}
return true
}There was a problem hiding this comment.
@gemini-code-assist This is incorrect. The only loop is over parts[1:] which is always bounded.
In the case of double stars you just end up with an iteration like so:
idx := strings.Index(s, "") == 0
remaining = remaining[0+0:]
remaining doesn't change that iteration, but the iteration continues
There was a problem hiding this comment.
That said, I'll add a commit to transform double star because it's a reasonable readability cleanup
This patch implements OIDC (kind of) for push authorization. It allows an ncps server to require and match OIDC identity tokens (such as those minted by CI systems including GitHub and Gitlab) against preconfigured sets of claims before accepting a PUT or DELETE. The existing "allow push" and "allow delete" configuration is still required and used as a coarse check. That is, a configuration with OIDC policies that does not *also* allow PUT and DELETE verbs will be rejected just as they are today without OIDC policies. If a server is not configured with any OIDC policies, this feature is a no-op for backwards compatibility. Identity tokens are extracted from Authorization headers. For compatibility with "nix store --to" (which typically uses .netrc files), the token can be read from "Basic" authorization. For example, a `.netrc` of: ``` machine ncps.mydomain.tld password <identity token> ``` Will produce a header like so: ``` Authorization: Basic <base64(username:password)> ``` In this case, the username would be empty and the password would be the token. This patch does not care what the username is. If "Basic" authorization is not used, this patch requires the token to be provided via "Bearer" authorization. NOTE: On startup ncps will reach out to all configured OIDC issuers to fetch their signing keys. If ncps cannot reach an issuer, or if the issuer is misconfigured, ncps will refuse to start up. While this places a startup time check on an ncps server, this is preferable to a runtime check that may not fail until a client attempts to write to the store. Because OIDC policy configuration is a list of maps it isn't configurable by CLI flags (urfave/cli-altsrc does not support this) and can only be specified in the configuration file.
avidal
force-pushed
the
avidal/oidc-authz
branch
from
e97cd05 to
0b3eea2
Compare
1 participant
See here for the relevant discussion.
This patch implements OIDC (kind of) for push authorization. It allows an ncps server to require and match OIDC identity tokens (such as those minted by CI systems including GitHub and Gitlab) against preconfigured sets of claims before accepting a PUT or DELETE.
The existing "allow push" and "allow delete" configuration is still required and used as a coarse check. That is, a configuration with OIDC policies that does not also allow PUT and DELETE verbs will be rejected just as they are today without OIDC policies.
If a server is not configured with any OIDC policies, this feature is a no-op for backwards compatibility.
Identity tokens are extracted from Authorization headers. For compatibility with "nix store --to" (which typically uses .netrc files), the token can be read from "Basic" authorization.
For example, a
.netrcof:Will produce a header like so:
In this case, the username would be empty and the password would be the token. This patch does not care what the username is.
If "Basic" authorization is not used, this patch requires the token to be provided via "Bearer" authorization.
NOTE: On startup ncps will reach out to all configured OIDC issuers to fetch their signing keys. If ncps cannot reach an issuer, or if the issuer is misconfigured, ncps will refuse to start up. While this places a startup time check on an ncps server, this is preferable to a runtime check that may not fail until a client attempts to write to the store.
Because OIDC policy configuration is a list of maps it isn't configurable by CLI flags (urfave/cli-altsrc does not support this) and can only be specified in the configuration file.