Improve basic auth handling for canonical ports - #15945
Conversation
openshift-merge-robot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: smarterclayton The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
openshift-merge-robot
added
the
approved
|
@legionus extracted from that other PR. Note the change to |
|
@openshift/sig-security |
|
@smarterclayton is this related to #14319? are you getting redirected to a URL you are not expecting? |
|
No, I just noticed it while testing the registry (at foo:443) was not using secrets for |
openshift-merge-robot
added
approved
|
PTAL |
openshift-merge-robot
added
the
needs-rebase
| {name: "mismatched scheme", args: args{keyring: fn("https://localhost", def), target: &url.URL{Scheme: "http", Host: "localhost"}}, user: "", password: ""}, | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { |
| {name: "canonical https", args: args{keyring: fn("localhost", def), target: &url.URL{Scheme: "https", Host: "localhost:443"}}, user: def.Username, password: def.Password}, | ||
| {name: "only https", args: args{keyring: fn("https://localhost", def), target: &url.URL{Host: "localhost"}}, user: def.Username, password: def.Password}, | ||
| {name: "only https scheme", args: args{keyring: fn("https://localhost", def), target: &url.URL{Scheme: "https", Host: "localhost"}}, user: def.Username, password: def.Password}, | ||
| {name: "mismatched scheme - http", args: args{keyring: fn("http://localhost", def), target: &url.URL{Scheme: "https", Host: "localhost"}}, user: def.Username, password: def.Password}, |
There was a problem hiding this comment.
Test cases with default and non-default ports in the keyring would be nice.
| if len(target.Scheme) == 0 || target.Scheme == "https" { | ||
| value = target.Host + target.Path | ||
| } else { | ||
| value = target.String() |
There was a problem hiding this comment.
Why http:// prefixed target is treated this way? It deserves at least a comment.
There was a problem hiding this comment.
We don't want to send https credentials to http endpoints. You have to explicitly say you want to send credentials to an insecure endpoint.
| {name: "only https scheme", args: args{keyring: fn("https://localhost", def), target: &url.URL{Scheme: "https", Host: "localhost"}}, user: def.Username, password: def.Password}, | ||
| {name: "mismatched scheme - http", args: args{keyring: fn("http://localhost", def), target: &url.URL{Scheme: "https", Host: "localhost"}}, user: def.Username, password: def.Password}, | ||
|
|
||
| // this is not allowed by the credential keyring, possibly because of insufficient testing |
There was a problem hiding this comment.
Maybe because https targets get trimmed before a lookup: 8ec0656#diff-d60fc09a78ae86f4931185b756b2ba00R168
There was a problem hiding this comment.
I meant that the upstream credential keyring is incorrectly handling http credentials.
There was a problem hiding this comment.
The upstream keyring cannot handle targets with scheme. If the "https" hadn't been removed from the target before a call to Lookup(), no match would had been found as well. The target shouldn't contain the scheme at all.
|
Any other comments? |
|
No, LGTM otherwise. |
8ec0656 to
e0ea61a
Compare
openshift-merge-robot
removed
the
needs-rebase
e0ea61a to
22c9886
Compare
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
Automatic merge from submit-queue |
7 participants
Also make it easier to set custom scopes (used in another PR) for direct registry access.