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 authentication against Artifactory #692

Merged

Conversation

imphil
Copy link
Contributor

@imphil imphil commented Nov 22, 2023

To authenticate against an OCI registry the code currently requires a
scope attribute in the WWW-Authenticate header. This key is optional
according to RFC 6750 [1], an empty string can be used if it's not
present.

Artifactory does not include the scope attribute and hence
authentication fails currently. Fix that by making the attribute
optional.

[1] "Use of the "scope" attribute is OPTIONAL."

Fixes #691

Copy link
Member

@samruddhikhandale samruddhikhandale left a comment

Choose a reason for hiding this comment

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

Thank you for taking the time in contributing this change, appreciate it! ✨

src/spec-configuration/httpOCIRegistry.ts Outdated Show resolved Hide resolved
Copy link
Member

@joshspicer joshspicer left a comment

Choose a reason for hiding this comment

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

Thanks for the change and pointer to the section in the RFC! One little change needed for the linter to pass, then it looks good to me 👍

I'll try to look into setting up a test Artifactory to add as a regression test here

To authenticate against an OCI registry the code currently requires a
`scope` attribute in the `WWW-Authenticate` header. This key is optional
according to RFC 6750 [1], an empty string can be used if it's not
present.

Artifactory does not include the `scope` attribute and hence
authentication fails currently. Fix that by making the attribute
optional.

[1] "Use of the "scope" attribute is OPTIONAL."

Fixes devcontainers#691
@imphil imphil force-pushed the auth-against-artifactory-scope branch from 98c90e7 to dc41f2f Compare November 27, 2023 19:02
@samruddhikhandale samruddhikhandale merged commit 34f35c2 into devcontainers:main Nov 27, 2023
19 checks passed
@imphil
Copy link
Contributor Author

imphil commented Nov 27, 2023

Thanks for your reviews and merging this PR! Just a quick question: this code is (I think) also used in the Dev Containers extension. What's roughly the expectation when this change will reach the extension?

@imphil
Copy link
Contributor Author

imphil commented Nov 27, 2023

I'll try to look into setting up a test Artifactory to add as a regression test here

One more note: Please feel free to reach out to me directly if you cannot reproduce the behavior I'm seeing. There seem to be a number of ways to set up Artifactory auth, so maybe I can dig up the right details to help you reproduce the problem.

@samruddhikhandale
Copy link
Member

Thanks for your reviews and merging this PR! Just a quick question: this code is (I think) also used in the Dev Containers extension. What's roughly the expectation when this change will reach the extension?

I can now look into releasing a new version of the @devcontainers/cli with these changes, and regarding Dev Containers Extensions @chrmarti might be able to provide a better timeline. But, it should be released soon enough.

@samruddhikhandale samruddhikhandale mentioned this pull request Nov 28, 2023
@samruddhikhandale
Copy link
Member

v0.54.1 of the @devcontainers/cli now includes this PR code changes 🚀

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.

"devcontainer features publish" fails to log in with Artifactory
3 participants