Skip to content
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: accept registry-scoped certfile and keyfile as credentials #5160

Merged
merged 1 commit into from Jul 20, 2022

Conversation

jenseng
Copy link
Contributor

@jenseng jenseng commented Jul 12, 2022

While this doesn't directly allow top-level cert/key as credentials (per the original issue), it's a more targeted/secure approach that accomplishes the same end-result; the new options are scoped to a specific registry, and the actual cert/key contents are much less likely to be exposed. See the RFC for more context.

References

Related to npm/rfcs#591
Depends on npm/npm-registry-fetch#125, npm/config#69
Closes #4765

@jenseng jenseng requested a review from a team as a code owner July 12, 2022 23:23
@wraithgar wraithgar self-assigned this Jul 13, 2022
@wraithgar
Copy link
Member

wraithgar commented Jul 18, 2022

Deps updates required for this pr: #5187

@jenseng jenseng force-pushed the registry-scoped-keyfile-and-certfile branch 2 times, most recently from 47ecd22 to 64ef34f Compare July 18, 2022 20:50
@jenseng
Copy link
Contributor Author

jenseng commented Jul 18, 2022

Inlined checks and rebased on to dep update branch. Thanks for the review, and let me know if anything else needs to change!

@jenseng jenseng force-pushed the registry-scoped-keyfile-and-certfile branch from 64ef34f to f110f65 Compare July 18, 2022 21:24
@wraithgar
Copy link
Member

Looks like coverage on those if statements is all that's remaining, and of course fixing up the branch once the deps PR lands.

@jenseng
Copy link
Contributor Author

jenseng commented Jul 18, 2022

coverage on those if statements

Slight chicken/egg problem in adding test coverage w/o rebasing atop the deps PR, as @npmcli/config@4.1.0 won't surface these new options. I'll rebase and push up the new tests once that deps PR lands

@jenseng jenseng force-pushed the registry-scoped-keyfile-and-certfile branch from f110f65 to eb1ea9c Compare July 19, 2022 20:09
@jenseng
Copy link
Contributor Author

jenseng commented Jul 19, 2022

Ok, added test coverage and rebased to get the deps PR

@wraithgar
Copy link
Member

looks like a make docs is in order. Normally that is supposed to happen automatically when you rebuild snapshots but it looks like that didn't happen. Manually running it should update docs/content/using-npm/config.md and then the docs CI task should go green.

Closes npm#4765
RFC: npm/rfcs#591

While this doesn't directly allow top-level cert/key as credentials (per the
original issue), it's a more targeted/secure approach that accomplishes the
same end-result; the new options are scoped to a specific registry, and the
actual cert/key contents are much less likely to be exposed. See the RFC for
more context.

Depends on:
* npm/npm-registry-fetch#125
* npm/config#69
@jenseng jenseng force-pushed the registry-scoped-keyfile-and-certfile branch from eb1ea9c to 9771bf6 Compare July 19, 2022 20:26
@fritzy fritzy merged commit 5ef53ee into npm:latest Jul 20, 2022
@fritzy fritzy mentioned this pull request Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] ENEEDAUTH when authenticating against a registry via mTLS
3 participants