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

fix: allow _auth to be set via environment #428

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

Conversation

shaneog
Copy link

@shaneog shaneog commented Nov 9, 2021

When using legacy auth, the env var NPM_CONFIG__AUTH can be used.

Fixes #324

@shaneog
Copy link
Author

shaneog commented Nov 9, 2021

I'm unsure why non-standard environment variables are used in this project (e.g. NPM_USERNAME versus NPM_CONFIG_USERNAME) but I've left them as is for now.

@cxn-mkalinovits
Copy link

Could somebody please check this pull request?

When using legacy auth, the env var `NPM_CONFIG__AUTH` can be used.

Fixes semantic-release#324
@shaneog shaneog force-pushed the fix/legacy-auth-env branch from 74804f3 to ddb390a Compare December 8, 2021 23:56
@shaneog
Copy link
Author

shaneog commented Dec 8, 2021

@gr2m or @travi - would you prefer a PR to make this repo use the standard convention for npm environment variables (in a backwards compatible way) rather than this PR?

@travi
Copy link
Member

travi commented Dec 9, 2021

I'm unsure why non-standard environment variables are used in this project (e.g. NPM_USERNAME versus NPM_CONFIG_USERNAME)

based on https://semantic-release.gitbook.io/semantic-release/support/faq#can-i-use-semantic-release-to-publish-a-package-on-artifactory, it looks like these were used to support legacy versions of artifcatory. this is a great example of why i'm hesitant to expand support for environment variables at all. i would rather remove than add additional complexity.

would you prefer a PR to make this repo use the standard convention for npm environment variables (in a backwards compatible way) rather than this PR?

i prefer the idea of aligning with the standard if we do proceed down this path, but before putting additional effort into an implementation, let's first decide whether it is a path that is worth pursuing for the project.

Just ran into the same issue because Artifactory uses NPM_CONFIG__AUTH or :_auth in .npmrc.

you mentioned this in the related issue, but I'm still trying to get my head around the need for this. i'm familiar with artifactory as a registry, but i don't understand what you mean by "Artifactory uses NPM_CONFIG__AUTH or :_auth in .npmrc". in my current use of artifactory, packages are published to artifactory from a CI pipeline. the auth details are needed from that CI pipeline, so i set NPM_TOKEN in that environment to a token with rights to publish to artifactory.

can you explain what you mean by "Artifactory uses NPM_CONFIG__AUTH or :_auth in .npmrc" and why only those options are available from where you are publishing from?

@shaneog
Copy link
Author

shaneog commented Dec 9, 2021

based on https://semantic-release.gitbook.io/semantic-release/support/faq#can-i-use-semantic-release-to-publish-a-package-on-artifactory, it looks like these were used to support legacy versions of artifcatory. this is a great example of why i'm hesitant to expand support for environment variables at all. i would rather remove than add additional complexity.

From my reading of the original PR it doesn't seem like the pre-existing NPM environment variables were considered. If @pvdlg is still around maybe he could chime in?

"Legacy auth" for npm is simply using username, password and email and doesn't define what the environment variables should be - they are defined separately and all prefixed with NPM_CONFIG_.

you mentioned this in the related issue, but I'm still trying to get my head around the need for this. i'm familiar with artifactory as a registry, but i don't understand what you mean by "Artifactory uses NPM_CONFIG__AUTH or :_auth in .npmrc". in my current use of artifactory, packages are published to artifactory from a CI pipeline. the auth details are needed from that CI pipeline, so i set NPM_TOKEN in that environment to a token with rights to publish to artifactory.

can you explain what you mean by "Artifactory uses NPM_CONFIG__AUTH or :_auth in .npmrc" and why only those options are available from where you are publishing from?

Apologies for the lack of clarity. Artifactory can use npm login (and thus a token) or Basic Authentication aka legacy. There are two ways to handle legacy/basic auth: either via username, password and email (which is converted into base64 in in this package here. Alternatively the base64 conversion can be performed outside semantic-release and provided as an _AUTH environment variable (i.e. NPM_CONFIG__AUTH). In my instance I don't have control of which to use, I can only use the provided environment variables that an Ops team provides in our CI. They have provided NPM_CONFIG__AUTH which works normally for all standard npm commands. However, since semantic-release does not support the standard environment variables, it does not work. This forced me to write out a .npmrc file manually before using semantic-release in our CI config. This is a hacky workaround and I'd prefer if semantic-release worked with the existing variables.

In other words, when legacy auth was added to semantic-release, it overlooked the fact that the _AUTH environment variable could be provided instead of the separate USERNAME and PASSWORD variables.
This PR fixes that oversight.

@travi
Copy link
Member

travi commented Dec 9, 2021

from both the original PR that you linked to and from the docs link that i included, the reason for supporting legacy auth was because of tools that did not yet support the use of tokens. that is no longer true for either tool that was mentioned. tokens should be preferred over un/pw at this point. the official registry no longer supports legacy auth at all

rather than expanding support for legacy auth, i would rather consider removing support from semantic-release altogether. this would encourage more secure registry interactions and simplify the implementation of this plugin.

what prevents you from convincing your ops team from making a token available to your builds. i recently worked through a similar situation for the artifactory instance that i work with and the improved security of using tokens helped with the decision.

@shaneog
Copy link
Author

shaneog commented Dec 9, 2021

from both the original PR that you linked to and from the docs link that i included, the reason for supporting legacy auth was because of tools that did not yet support the use of tokens. that is no longer true for either tool that was mentioned. tokens should be preferred over un/pw at this point. the official registry no longer supports legacy auth at all

Does that mean that those organizations that do not use the official registry, or who use a registry without token support are left out in the cold?

rather than expanding support for legacy auth, i would rather consider removing support from semantic-release altogether. this would encourage more secure registry interactions and simplify the implementation of this plugin.

For the general development ecosystem, I agree with you. However, for encouraging the use of semantic-release I disagree since it would prevent people adopting semantic-release that otherwise could.
For better or worse, there exists organizations where the internal infrastructure/tooling pace of change is limited, and I don't believe that developers who work in such organizations should be penalized from using tools such as semantic-release.

what prevents you from convincing your ops team from making a token available to your builds. i recently worked through a similar situation for the artifactory instance that i work with and the improved security of using tokens helped with the decision.

This is in progress, but larger organizations tend to have some inertia to overcome, I would prefer to use semantic-release today.


I guess it comes down to the goal of this library. Either it lightly wraps npm to add some semantic-release specific config in which case it should support all that npm supports (this is my current understanding); OR, it supports only a very specific subset of what npm supports based on the maintainers outlook (which is perfectly fine, as long as it's defined and declared as such).
My understanding of this library is based on it being the official way of using npm for semantic-release, as opposed to a way of using npm for semantic-release in which case I would not expect it to live under the semantic-release org.

@travi
Copy link
Member

travi commented Dec 9, 2021

Does that mean that those organizations that do not use the official registry, or who use a registry without token support are left out in the cold?

the intention is not to leave anyone out in the cold because of limitations of supported tooling. however, artifactory does support using tokens, but your organization has chosen to only enable the less-secure legacy auth approach at this point. i am not aware of any alternative npm registries that still do not support token auth at this point.

this problem is solvable by your organization (although i do understand how difficult it can be to influence such changes and have battled similar situations myself).

Either it lightly wraps npm to add some semantic-release specific config in which case it should support all that npm supports (this is my current understanding); OR, it supports only a very specific subset of what npm supports based on the maintainers outlook (which is perfectly fine, as long as it's defined and declared as such).

"npm" can refer to multiple things. the "npm" registry no longer supports legacy auth. how much of the "npm" cli we lightly wrap is a decision that we do have to consider while maintaining the project, but it is not as simple assuming that only wrapping is always the best answer. we do prefer to use an existing standard implemented by the npm cli when it accomplishes the goals of the project. however, the core reason that semantic-release even exists is to encourage better practices than the raw npm cli does on its own.

there will be a point when legacy auth is dropped, even by the npm cli. i'm simply attempting to balance trade-offs to think through what the best steps are for this project.

  • it sounds like you have a workaround that works while you wait for proper tokens to be available, even though it may be a bit hacky
  • we are a very small team of volunteer maintainers working on semantic-release for free in our spare time. we need to consider how added complexity impacts the long-term sustainability of the project
  • adding support for the proposed approach in a non-breaking way would be additive in terms of complexity and details that we have to maintain going forward
  • switching the existing use of environment variables to the more standard approach without maintaining backwards compatibility might be equivalent complexity to maintain, but would be a breaking change
  • if we are going to implement a breaking change around how we enable auth, it is worth us considering whether that would be the time to remove support for discouraged options. (i'm not saying we are going to do this now, but it should be considered if we want to reduce complexity when releasing a related breaking change)

i understand the situation you are in, but when considering the ongoing maintenance and encouraging good practices, i'm not convinced that this change deserves our attention over more pressing maintenance needs when you have a workaround and a path toward moving to the more appropriate use of tokens. i would be more interested in a similar change related to environment variables not related to legacy auth, but even alternatively named env vars could be handled like NPM_TOKEN=$OTHER_NAME npx semantic-release in the meantime.

if there are documentation improvements that you think could better encourage the approach of using tokens over legacy auth, even to help explain the situation to ops teams of others in similar situations, we would appreciate suggestions as a PR.

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.

Allow running without an configured NPM_TOKEN
3 participants