Add support for Basic Authentication to proxyingRegistry
#4263
Conversation
0c60866 to
a6f5498
Compare
|
Could someone help me finding out why these CodeQL are triggered? I don't see it. |
a680eee to
800a567
Compare
|
Looking at the codeql, you haven't introduced any new issues, the issue already exists in the repo. Because you're editing the code where the issue exists, codeql is flagging this pr. Is there a reason you're renaming the struct? You might get away with it if you don't :) |
Good idea, let's see if it helps 😅 |
|
|
Signed-off-by: oliver-goetz <o.goetz@sap.com>
f5b476d to
1e8ea03
Compare
@milosgajdos can you help retesting the above failing check? Can you also help reviewing this PR? Thanks in advance! |
|
Yeah, I've no idea if these are legit or false positives. These warnings are about logging sensitive data -- I think we "hydrate" context with all kinds of garbage in this project, so I'd have to spend some time and trace/investigate these, but I'm short of time nowadays, so you'll have to wait unless one of 10+ other maintainers have more time |
|
My guess here is that the app struct embeds the context instead of using it as a field, so the context/app are mingled. As app contains sensitive data, CodeQL assumes the context does too. Having a look at breaking it into a field to see if it cleans up the codeql errors. Edit: Nope, not that. |
|
Having had a really good look through and played with a couple of options for removing the errors, such as removing a whole load of stuff from the context, I feel its really a false positive. I've raised that here: github/codeql#16486 I suggest we go ahead and merge this. |
I'm wondering if we should mark these as false positives as they'll probably keep cropping up on all the subsequent PRs potentially confusing contribuors |
|
Only when this code is touched, and I'd like to see what the codeql team say about them first. They may have insight that I've missed. |
This PR adds support for
Basic Authenticationto registry proxy.Before it supported Bearer tokens only. However, basic auth was already almost implemented as there was already an appropriate
AuthenticationHandlerused to obtain Bearer tokens.Fixes #3153