-
Notifications
You must be signed in to change notification settings - Fork 248
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
Implement spin registry login
#1150
Conversation
@@ -7,6 +7,8 @@ edition = { workspace = true } | |||
[dependencies] | |||
anyhow = "1.0" | |||
bindle = { workspace = true } | |||
base64 = "0.21.0" | |||
dkregistry = { git = "https://github.com/camallo/dkregistry-rs", rev = "37acecb4b8139dd1b1cc83795442f94f90e1ffc5" } |
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.
The latest published version of this crate uses Tokio 0.18, which makes it incompatible with our existing setup.
The functionality used from that crate is minimal (only making an authenticated request to the registry using the credentials, to validate them). If we consider the git source a blocker, we can:
a) remove the functionality altogether — I'm opposed to this, since it catches user errors early
b) fork the repo and publish a new version ourselves
c) update the existing OCI client library to do this instead.
I'm in favor of c).
|
||
// Save an encoded representation of the credential set in the local configuration file. | ||
let mut auth = AuthConfig::load_default().await?; | ||
auth.insert(server, username, password)?; |
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.
I know this is "how its done" but WHAT YEAR IS THIS?!?!
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.
ref #940
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.
I'm mostly complaining about storing passwords rather than tokens, but thats a good point too.
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.
Yeah, we need to address both of them..
impl AuthConfig { | ||
/// Load the authentication configuration from the default location | ||
/// ($XDG_CONFIG_HOME/fermyon/registry-auth.json). | ||
pub async fn load_default() -> Result<Self> { |
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.
Do we want this path to be configurable?
I would really like us to refactor the way we handle configuration throughout Spin, and have a central place where we handle reading and writing configuration...
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.
Yeah I've been thinking about how to centralize this sort of thing. We could call it spin-config
no wait... spin-core
! no.... spin-app
? shoot...
4718832
to
3170a39
Compare
crates/publish/src/oci/client.rs
Outdated
// TODO: add a way to override this path. | ||
let default_path = dirs::config_dir() | ||
.context("Cannot find configuration directory")? | ||
.join("fermyon") |
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.
Shouldn't this be "spin"?
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.
This is where the Cloud auth file is.. really don't want to split to yet another directory before we consolidate..
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.
Well, I think that's where it ultimately belongs and we'll have to do some kind of migration if we want to move it later...
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.
When we moved things to the fermyon
, we talked about potentially having multiple Fermyon tools sharing that configuration.
I still think that is a good idea, so moving them under spin
wouldn't really make sense. That being said, without a proposal for consolidating configuration..
1e12b56
to
e759e09
Compare
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.
I think it's mostly nits and suggestions - if you just want to land it, then ping me and I can approve instead.
) -> Result<()> { | ||
let client = dkregistry::v2::Client::configure() | ||
.registry(server.as_ref()) | ||
.insecure_registry(false) |
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.
No --insecure
option?
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.
Following the UX of the Docker CLI for login (https://docs.docker.com/engine/reference/commandline/login), there is no way to authenticate against an insecure registry. You can use it anonymously to run from, but you should not be able to send credentials to it.
cdc0510
to
a863642
Compare
I addressed most of your comments, @itowlson. |
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!
This commit implements functionality for logging in to an OCI registry. Until this commit, a user would have had to use external tooling to log in to a target registry (such as `docker login` or other registry specific tools). This had the advantage that users who already had a setup for working with container regsitries could continue to use it, but it introduced a dependency on external tools for pushing a Spin application. This commit implements the functionality to login to a registry natively into Spin. Specifically, it adds this a `spin regsitry login` command: ``` $ spin registry login --help Log in to a registry USAGE: spin registry login [OPTIONS] <SERVER> ARGS: <SERVER> OPTIONS: -h, --help Print help information -p, --password <PASSWORD> Password for the container registry --password-stdin Take the password from stdin -u, --username <USERNAME> Username for the container registry ``` This command mirrors existing tools that perform login operations to registries (flags for the username and passwords, flag to read the password from standard in that takes precedence). The credentials are base64 encoded (standard for container tools that do not use Docker's credential helpers) and written to `$XDG_CONFIG_HOME/fermyon/registry-auth.json` (side-by-side with the Fermyon Cloud authentication file). Functionality that needs to work with a registry (`spin registry push/pull`, `spin up`) will first search the Spin configuration for the credentials, then will fallback to searching the authenticated registries in the Docker configuration file. Examples: ``` $ echo $GHCR_PAT | spin registry login --username radu-matei ghcr.io --password-stdin Successfully authenticated as radu-matei to registry ghcr.io $ spin registry login --username radumatei --password <password> https://index.docker.io Successfully authenticated as radumatei to registry https://index.docker.io $ spin registry login index.docker.io Username: radumatei Password: (prompt, hidden input) Successfully authenticated as radumatei to registry https://index.docker.io ``` This functionality should make `spin registry login` a drop-in replacement for most of the existing tools that login to registries, while continuing to reuse existing credentials as a fallback. A future commit needs to make writing the configuration file more flexible, and add unit tests for decoding credentials. Signed-off-by: Radu Matei <radu.matei@fermyon.com>
a863642
to
e3bfed7
Compare
close #1087
This commit implements functionality for logging in to an OCI registry. Until this commit, a user would have had to use external tooling to log in to a target registry (such as
docker login
or other registry specific tools). This had the advantage that users who already had a setup for working with container regsitries could continue to use it, but it introduced a dependency on external tools for pushing a Spin application.This commit implements the functionality to login to a registry natively into Spin.
Specifically, it adds this a
spin regsitry login
command:This command mirrors existing tools that perform login operations to registries (flags for the username and passwords, flag to read the password from standard in that takes precedence).
The credentials are base64 encoded (standard for container tools that do not use Docker's credential helpers) and written to
$XDG_CONFIG_HOME/fermyon/registry-auth.json
(side-by-side with the Fermyon Cloud authentication file).Functionality that needs to work with a registry (
spin registry push/pull
,spin up
) will first search the Spin configuration for the credentials, then will fallback to searching the authenticated registries in the Docker configuration file.Examples:
This functionality should make
spin registry login
a drop-in replacement for most of the existing tools that login to registries, while continuing to reuse existing credentials as a fallback.A future commit needs to make writing the configuration file more flexible, and add unit tests for decoding credentials.