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

RFC: Registry-scoped keyfile / certfile credential options #591

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jenseng
Copy link

@jenseng jenseng commented May 20, 2022

@darcyclarke darcyclarke added the Agenda will be discussed at the Open RFC call label May 25, 2022
1. **`npm-registry-fetch`**: Update [`getAuth` + `Auth`](https://github.com/npm/npm-registry-fetch/blob/main/lib/auth.js) to resolve any scoped `certfile` and `keyfile` options into corresponding `cert` and `key` properties, and update [`regFetch`](https://github.com/npm/npm-registry-fetch/blob/main/lib/index.js#L26) to use them in the `fetch` call.
2. Accept `certfile`/`keyfile` as valid creds, i.e.
1. **`@npmcli/config`**: Update [`getCredentialsByURI`](https://github.com/npm/config/blob/1244177d8a68f68f2be46d5b9c21957da7dedfbb/lib/index.js#L759) to process scoped `certfile`/`keyfile` options and then set them in the returned credentials.
2. **`npm`**: Update [this `noCreds` check](https://github.com/npm/cli/blob/e992b4a21ecdd96aa33c59682c0ac0cc8a30d776/lib/commands/publish.js#L108) to accept `certfile`/`keyfile` as creds.
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Digging a little deeper, looks like cli:lib/utils/get-identity.js and friends may need to be updated too, as there are some assumptions around it being possible to get the current username, which would not necessarily be the case if authn is strictly done via mTLS 🤔 .

If that ends up being a whole can of worms, maybe this RFC could be reduced in scope (pun!) so as to cover just registry-scoped certfile/keyfile, leaving out the only-authenticate-via-mTLS feature. Then (I think) the changes can be limited to just npm-registry-fetch.

Copy link
Author

@jenseng jenseng May 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO though it is reasonable to require custom registries to implement a /-/whoami for compatibility, in which case a meaningful username could be returned by get-identity, so that commands like unpublish still work when authenticating solely via mTLS

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "fail if you're not logged in" checks are a UX thing, and it's probably ok to err on the side of trying the attempt if we have a keyfile.

jenseng added a commit to jenseng/npm-registry-fetch that referenced this pull request Jul 12, 2022
Closes npm#118
RFC: npm/rfcs#591

Add support for registry-scoped certfile and keyfile options, e.g.

```
{
  "//my.registry.example/npm/:certfile": "~/.secret/stuff.crt",
  "//my.registry.example/npm/:keyfile": "~/.secret/stuff.key"
}
```

Since these are registry-specific, they will override top-level cert and
key options (if set).

Like the top-level `cafile` option, these registry-scoped options are
silently ignored if invalid.
jenseng added a commit to jenseng/config that referenced this pull request Jul 12, 2022
RFC: npm/rfcs#591

See also: npm/npm-registry-fetch#125

By itself this change doesn't do much, but it enables us to resolve
npm/cli#4765 and surface these options anywhere
else they may be needed.
jenseng added a commit to jenseng/cli that referenced this pull request Jul 12, 2022
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
wraithgar pushed a commit to npm/config that referenced this pull request Jul 18, 2022
RFC: npm/rfcs#591

See also: npm/npm-registry-fetch#125

By itself this change doesn't do much, but it enables us to resolve
npm/cli#4765 and surface these options anywhere
else they may be needed.
jenseng added a commit to jenseng/npm-registry-fetch that referenced this pull request Jul 18, 2022
Closes npm#118
RFC: npm/rfcs#591

Add support for registry-scoped certfile and keyfile options, e.g.

```
{
  "//my.registry.example/npm/:certfile": "~/.secret/stuff.crt",
  "//my.registry.example/npm/:keyfile": "~/.secret/stuff.key"
}
```

Since these are registry-specific, they will override top-level cert and
key options (if set).

Like the top-level `cafile` option, these registry-scoped options are
silently ignored if invalid.
wraithgar pushed a commit to npm/npm-registry-fetch that referenced this pull request Jul 18, 2022
Closes #118
RFC: npm/rfcs#591

Add support for registry-scoped certfile and keyfile options, e.g.

```
{
  "//my.registry.example/npm/:certfile": "~/.secret/stuff.crt",
  "//my.registry.example/npm/:keyfile": "~/.secret/stuff.key"
}
```

Since these are registry-specific, they will override top-level cert and
key options (if set).

Like the top-level `cafile` option, these registry-scoped options are
silently ignored if invalid.
jenseng added a commit to jenseng/cli that referenced this pull request Jul 18, 2022
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 added a commit to jenseng/cli that referenced this pull request Jul 18, 2022
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 added a commit to jenseng/cli that referenced this pull request Jul 18, 2022
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 added a commit to jenseng/cli that referenced this pull request Jul 19, 2022
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 added a commit to jenseng/cli that referenced this pull request Jul 19, 2022
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
fritzy pushed a commit to npm/cli that referenced this pull request Jul 20, 2022
Closes #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
@darcyclarke darcyclarke added ratify and removed Agenda will be discussed at the Open RFC call labels Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants