-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat: Allow configuring OSS Index user/pw directly #7640
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
feat: Allow configuring OSS Index user/pw directly #7640
Conversation
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.
LGTM
Only thing I would say is take a look at Hans' comment #7631 (comment) regarding environment variables. Thoughts? |
I had considered it, and of course it makes technical sense. However, I am still undecided as to whether the additional complexity is justified. Do we expect people to run |
Hi @marcelstoer 👋 Thank you for your proposal. No worries, I can discard my PR so we focus on iterating on yours, I stalled my work on it. |
Regarding the |
@jeremylong @aikebah I feel we should do this hiding-sensitive-information-through-the-use-of-env-variables consistently across the project. Use the same coding pattern, naming scheme plus documentation for all affected parameters. I don't think it'd be helpful if I just followed what we have for the NVD API key and applied it for the OSS Index user/pw. From what I can tell, the NVD API key is the only parameter so far that we treat like this? If I counted correctly, we have 7 '*password' and '*token' parameters, 8 for '*user', and 2 for '*key'. Hence, 24 - 1 new 'xxxEnvironmentVariable' parameters. → more unrelated refactoring than should be included in this PR |
@marcelstoer I agree that it would be good to use same strategy for other cases of passwords information, however I consider those separate enhancements of existing functionality, so they could be taken care of outside the scope of this change-request which is all about enabling an alternative to communicate OSSIndex credentials to the Maven plugin. Most user/pass settings are for on-prem, typically corporate, copies or reverse-proxies exposing open public information behind a login (typically due to corporate policies demanding 'only authenticated access on any system', even when it only exposes public data in mirrored form internally in the company infrastructure). Their usage is expected to be the exceptional case, whereas NVD API and OSSIndex logins are to be expected a regular case as they are a means to obtain a higher max request rate on publicly available resources. |
I'll open an enhancement request around secrets/creds as env variables. |
Description of Change
When both
ossIndexUsername
andossIndexPassword
are provided, we don't evaluateossIndexServerId
.Related issues
Fixes #7482
Have test cases been added to cover the new functionality?
Only manual testing