fix: OIDC issuerCA to load all certificates from PEM bundle#874
fix: OIDC issuerCA to load all certificates from PEM bundle#874JoaoBraveCoding wants to merge 2 commits intoobservatorium:mainfrom
Conversation
philipgough
left a comment
philipgough
left a comment
There was a problem hiding this comment.
/lgtm
@squat any concerns?
|
Is there anything else I can help with to get this one merged? |
squat
left a comment
squat
left a comment
There was a problem hiding this comment.
I'm find with this functionality. I just have one style comment to improve clarity and prevent future mistakes with validation.
|
@squat thanks for the reply! However, I don't think your comment went through |
authentication/oidc.go
Outdated
| for { | ||
| block, rest = pem.Decode(rest) | ||
| if block == nil { | ||
| if len(config.issuerCAs) == 0 { |
There was a problem hiding this comment.
For style and clarity, let's instead break on line 107, ie, whenever block == nil since that's the exit condition and let's move this check outside the block, since that's the validation. I think that would be clearer IMO.
|
@JoaoBraveCoding oops. I just reposted! |
|
Please remove that last commit. We can make it a separate PR and merge before this of necessary but it's unrelated |
JoaoBraveCoding
force-pushed
the
LOG-8727
branch
from
330abe6 to
722a125
Compare
|
@squat thank you for that, you are right:
|
3 participants
Solves https://issues.redhat.com/browse/LOG-8727