-
Notifications
You must be signed in to change notification settings - Fork 87
chore: bump Go to 1.25.8 and fix CI and gosec issues #378
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
maxlambrecht
wants to merge
4
commits into
spiffe:main
Choose a base branch
from
maxlambrecht:chore/update-golangci-lint-gosec
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
a287d95
fix: update golangci-lint and address gosec findings
maxlambrecht 872be4c
chore: bump minimum Go version to 1.25.8
maxlambrecht 6c7af92
fix: use full Go version in Makefile download URLs
maxlambrecht 79fa547
docs: clarify VerifyConnection migration and compatibility notes
maxlambrecht File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| module github.com/spiffe/go-spiffe/v2 | ||
|
|
||
| go 1.24.0 | ||
| go 1.25.8 | ||
|
|
||
| require ( | ||
| github.com/Microsoft/go-winio v0.6.2 | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm worried about backwards compatibility. There are two situations:
I need to think about how we handle this. Open to suggestions. It may be the case that we have to accept a small breaking change. In any case, whatever we pick, the nuances should be documented on the functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. The implementation already preserves the input-side case by capturing any existing
config.VerifyPeerCertificate/config.VerifyConnectioncallbacks and invoking them after SPIFFE auth succeeds. The remaining change is the narrow returned-config case: callers that previously wrappedVerifyPeerCertificateon the returned config now need to extendVerifyConnectioninstead, since SPIFFE auth has to live onVerifyConnectionto also run on resumed sessions. I updated the function docs to call out that nuance explicitly, and I added focused tests for the preserved input-callback behavior and chaining/error propagation. So this is a narrow breaking change, but I think it is the right tradeoff here.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to throw my 2c into the mix:
About a month or two back, I had some interesting conversations with folks on the topic of TLS resumption. My original stance had been that it made sense to re-verify certificates upon resumption of a connection but I did hear a strong counter argument that has made me somewhat reconsider.
The argument goes that if you are not continually monitoring open TLS connections and dropping them if they are no longer "valid" (i.e CA has rotated, or cert has expired), then what makes a resumption event particularly remarkable that you need to re-perform this verification?
Curious on others thoughts on this since I haven't quite made my mind up either way.
Another element came to mind for me as well - which perhaps @maxlambrecht may be better suited to answer in regards to: In Go, the peer certificate is stored as part of the session state/ticket and allows us to re-verify upon resumption. Is this the case for Rust/other languages as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Rust, I think this is more about the TLS library than the language itself. In
rustls,peer_certificates()is available for both full and resumed handshakes, so the peer certificate chain is still available after resumption. But at least on the TLS 1.3 client resumption path, rustls does not re-run the full certificate verification. It treats resumption as a continuation of the previous session and restores the cached server certificate chain into the connection state.So for
rustls, my understanding is: yes, peer cert material is available on resume, but no, that does not mean the chain is verified again from scratch on that path.For Java and Python, I think the general idea is similar, but it may depend on the TLS implementation. I have not verified the exact behavior there in enough detail to say more confidently.