re-organize CLI config files #727

ahl · 2024-07-08T16:54:18Z

This moves us from a hosts.toml indexed by the "host" used to connect to the Oxide API server to a "profile"-based approach. Profile data will span the config.toml and credentials.toml, the former being primarily user-managed, the latter CLI/auth managed. To use a profile, users will specify --profile XXX; there is a default-profile set in config.toml. Specifying the profile rather than the host will, I expect, be much more user friendly.

Note that this also simplifies the SDK <-> CLI boundary. We add the credentials.toml configuration mechanism to the SDK so that SDK consumers can use that same authentication information. But we move the CLI-specific context information back to the CLI (since it's not really a generally useful mechanism appropriate for the SDK).

Closes #6, #163, #250, #254, #301, #391, #508, #737.

- rip out old config stuff - only create the Client when we need it

- test the auth commands under various conditions

david-crespo · 2024-07-08T17:36:14Z

We talked about this in DMs, but to sum up: I was skeptical of the need to have the SDK be able to read from the same credentials config files as the CLI, because I am used to SDKs having a more explicit mechanism for specifying credentials. Usually I see env vars plus client constructor params for passing in a token (and in our case a host). The TS and Go SDKs already have the constructor params. However, seeing that both the GCP and AWS client libraries support authing from the credentials set up by their respective CLIs, I concede it's probably useful and users may expect this from us.

So, my suggestions were:

Let's make sure we explain the different ways of authing and how to opt in or out
Add explicit host and token params to the SDK client constructor so the user can take matters fully into their own hands if they want (and if these params are used, do not fall back to any other mechanism)

ahl · 2024-07-08T22:48:06Z

@david-crespo I added the raw token auth method; still more work needed, but thanks for the suggestion.

karencfv

Nice! I'll be able to have the terraform provider use the profiles as well when this is merged. This will really improve UX all around :)

karencfv · 2024-07-08T23:38:00Z

cli/src/cmd_auth.rs

+    // TODO seems pointless
    /// Read token from standard input.
    #[clap(long)]
    with_token: bool,


Sadly, we can't stop people from grabbing a token from the credentials file of a machine and pasting it to the credentials file of another. By passing the token they already have through this flag we can deter them from manually editing the credentials file.

karencfv · 2024-07-08T23:43:05Z

cli/src/cmd_auth.rs

+            // TODO what's the point of this?
+            let client_id = Uuid::new_v4();


I think this was initially written by @plotnick , I'm pretty sure there was a good reason

Reconstructing from distant memories and a quick perusal of the spec... We don't currently assign clients any kind of unique identifier, so a random UUID is about the best we can do here. It's not holding any weight, but it's not optional in the OAuth protocol.

Yeah. The question was to myself. I saw its use by the oauth dance and wanted to investigate if there's something meaningful here or if we should document it in the code as unimportant.

karencfv · 2024-07-08T23:48:00Z

cli/src/cmd_auth.rs

+        std::fs::write(credentials_path, credentials.to_string())
+            .expect("unable to write credentials.toml");
+
+        println!("{}", credentials);


Could you add a comment at the start of the file warning users not to manually edit this file?

cli/src/cmd_auth.rs

ahl · 2024-07-09T03:52:08Z

@karencfv I'll be happy to let you know when this is ready for review.

augustuswm · 2024-07-12T19:36:19Z

In good news, updating to this version worked without any changes for cassette. It primarily uses Client::new_with_client since it needs to abstract over api tokens and session cookies (I think using session cookies should always require a custom client). The CliConfig struct makes sense to me for a new method of passing credentials.

I'm not sure about opting in to reading files by default. I don't think it is terribly onerous to ask the caller to use Client::authenticated(&CliConfig::default().with_credentials("path-to-credentials")). Or to follow something like GCP, allow the path to be specified by an ENV variable (https://cloud.google.com/docs/authentication/application-default-credentials). With a call opting in Client::authenticated(&CliConfig::default().with_env()).

cli/README.md

ahl · 2024-07-13T00:12:47Z

cli/src/cmd_auth.rs

-///
-/// Alternatively, pass in a token on standard input by using `--with-token`.
-///
-///     # start interactive setup
-///     $ oxide auth login
+/// Authenticate with an Oxide Silo
 ///
-///     # authenticate against a specific Oxide instance by reading the token
-///     # from a file
-///     $ oxide auth login --with-token --host oxide.internal < mytoken.txt
-///
-///     # authenticate with a specific Oxide instance
+///     # authenticate with a specific Oxide silo
 ///     $ oxide auth login --host oxide.internal
 ///
-///     # authenticate with an insecure Oxide instance (not recommended)
+///     # authenticate with an insecure Oxide silo (not recommended)
 ///     $ oxide auth login --host http://oxide.internal


@twinfees My hope is that this fixes #737

ahl · 2024-07-13T00:24:43Z

I'm not sure about opting in to reading files by default. I don't think it is terribly onerous to ask the caller to use Client::authenticated(&CliConfig::default().with_credentials("path-to-credentials")). Or to follow something like GCP, allow the path to be specified by an ENV variable (https://cloud.google.com/docs/authentication/application-default-credentials). With a call opting in Client::authenticated(&CliConfig::default().with_env()).

I hear you, but I really wanted to make it easy to build a custom consumer. For example, if we know where creds are kept, why not make that the default? Perhaps put another way, why unintentional use might we see happening that would create friction for users of the SDK (or CLI)?

ahl · 2024-07-13T00:32:42Z

@karencfv and @david-crespo this is ready for your review. I see this as the foundation to fix a few outstanding issues:

support named "profiles" #6
tabular output #25 by having output format specified in config.toml
Canonicalize host #163 (more or less obviates that)
Communication Error: builder error: relative URL without a base #250
Add disk import logic to SDK #255 (now that Context is cleaned up)
Facility for SDK consumer to authenticate using CLI creds #254
If OXIDE_HOST is not set, we choose a random host from the user's config #301
project list should not panic if the client is not authenticated #391
Small complaints about auth subcommand output #508
oxide auth status looks like it's hanging when it isn't #567

....etc

david-crespo · 2024-07-13T00:32:46Z

Maybe you could split the difference by making it opt-in (through a special constructor or whatever) to read from the default location, but you don’t have to give a path — it would know.

I admit that the Bad Thing (reading from the CLI config files when you don’t want to) is relatively unlikely, because if someone wants to specify creds they will probably do so and override that default behavior. It still feels a little weird to me for a library (as opposed to a CLI) to do something like that.

ahl · 2024-07-13T00:35:38Z

Maybe you could split the difference by making it opt-in (through a special constructor or whatever) to read from the default location, but you don’t have to give a path — it would know.

I think a common use case is going to be 1. oxide auth login 2. build a custom tool that uses those creds. I could have a ClientConfig::new() that didn't include defaults... but that doesn't strike me as easier or clearer. What problem would we see explicit opt-in as solving?

cli/README.md

cli/docs/cli.json

karencfv · 2024-07-14T23:32:18Z

cli/docs/cli.json

@@ -79,8 +84,8 @@
      "subcommands": [
        {
          "name": "login",
-          "about": "Authenticate with an Oxide host.",
-          "long_about": "Authenticate with an Oxide host.\n\nAlternatively, pass in a token on standard input by using `--with-token`.\n\n    # start interactive setup\n    $ oxide auth login\n\n    # authenticate against a specific Oxide instance by reading the token\n    # from a file\n    $ oxide auth login --with-token --host oxide.internal < mytoken.txt\n\n    # authenticate with a specific Oxide instance\n    $ oxide auth login --host oxide.internal\n\n    # authenticate with an insecure Oxide instance (not recommended)\n    $ oxide auth login --host http://oxide.internal",
+          "about": "Authenticate with an Oxide Silo",


Will the average dev/SRE user know what a silo is? I was under the impression the term "silo" was mostly hidden from them and only really relevant to operators.

I think Silo is a term that we're educating users about. Certainly it's more precise than a "host" ... which is a term of whose definition I'm very unsure.

Yeah, this is a tough one. In the web console, we generally say that for the developer user without fleet permissions, the silo is their universe and we try to avoid talking about the silo. However, we do say it right in the top left corner, and (as of recently) we call silo images "silo images". So it's not completely hidden.

Notably silos are also the first thing in the key concepts guide. So I think we've definitely softened the "first rule of silos is never talk about silos" idea.

https://docs.oxide.computer/guides/key-entities-and-concepts

karencfv · 2024-07-14T23:40:27Z

cli/src/cmd_auth.rs

-    /// Read token from standard input.
-    #[clap(long)]
-    with_token: bool,


Is this not something we want for users that already have a token so they don't manually edit the credentials file?

I think my aversion is more to unused- or seldom-used functionality, and I don't have the same aversion as I infer you do to people editing config files.

karencfv · 2024-07-14T23:43:52Z

cli/src/cmd_auth.rs

+            // If the profile already has a token, alert the user.
+            if ctx.cred_file().profile.contains_key(profile_name)
+                && !yes(format!(
+                    "The profile \"{}\" is already authenticated. {}",
+                    profile_name, "Do you want to re-authenticate?",
+                ))?
+            {


Are tokens revoked when a user re-authenticates? I may be wrong but I think they will still be valid :/
If that's the case should we even allow re-authentication? Users may have a false sense of security and think that by re-authenticating the previous token will be revoked automatically.

If I'm wrong, just dismiss this comment :)

Note that we haven't checked the validity of the credentials at this point. It may be that people want to re-authenticate because the existing creds are invalid (... for reasons?). Note that this is preserving existing functionality; if we want to change it, could we do so outside the scope of this PR?

karencfv · 2024-07-14T23:47:26Z

cli/src/cmd_auth.rs

    /// The host of the Oxide instance to authenticate with.
    /// This assumes https; specify an `http://` prefix if needed.
    #[clap(short = 'H', long, value_parser = parse_host)]
    host: url::Url,

    /// Override the default browser when opening the authentication URL.
-    #[clap(long, group = "browser-options")]
+    #[clap(long)]


Would it be useful to have a flag to optionally set this profile as default?

Maybe! Can we do that later?

karencfv · 2024-07-14T23:58:27Z

cli/src/cmd_auth.rs

+            .filter_map(|(name, info)| (info.host == self.host.as_str()).then_some(name))
+            .next()
+        {
+            // If the host is already present in a profile, alert the user.


Maybe we can also handle the case where the user chose a profile name that already exists?

I'm not sure I follow; wouldn't that lead us down the path where we ask if the user wants to re-authenticate?

karencfv · 2024-07-15T00:05:07Z

cli/src/cmd_auth.rs

-                }
-            }
+    pub async fn logout(&self, ctx: &Context) -> Result<()> {
+        if !self.force && !yes("Confirm authentication information deletion:")? {


It'd be nice to confirm that the user is deleting the profile they intend to delete. How about

"Confirm authentication information deletion for profile {PROFILE NAME}:"

karencfv · 2024-07-15T00:06:58Z

cli/src/cmd_auth.rs

+            let Some(profile_name) = profile else {
+                bail!("No profile specified and no default profile");
+            };


Does this mean that if no profile name is selected, it'll just remove information of the default profile? Maybe we could add a warning or something? Or disallow logging out without setting the profile to be logged out of?

Does this PR change things in that regard?

karencfv · 2024-07-15T00:10:48Z

cli/src/cmd_auth.rs

                }
            };
+        } else {
+            for (profile_name, profile_info) in &ctx.cred_file().profile {


It'd be nice to have a way to check the status of a single profile. WDYT?

Sure! What do you propose? Can we do it as a follow-on?

augustuswm · 2024-07-15T15:36:24Z

I do agree that using the CLI to create a token and then using the SDK is a common use case today. Some of my hesitance is that the current CLI authentication feels more of a result of a limitation in the product (i.e. device OAuth is the only method to create tokens). It also add profiles as a SDK concept, which then asks how can profiles be managed via the SDK instead of files (though we could add this later).

It may also just be me being paranoid, but something doesn't feel correct about an SDK that reads all of the credentials in a config file (even if AWS does it) when they may not be relevant to the executing program. I think Google handles this model a lot better, in that it requests that the user explicitly create an Application Default Credentials file (via a CLI command) which encodes a single credential. Their SDKs then load this file from an expected location.

ahl · 2024-07-15T16:06:28Z

It may also just be me being paranoid, but something doesn't feel correct about an SDK that reads all of the credentials in a config file (even if AWS does it) when they may not be relevant to the executing program.

I confess that I don't fully understand your concern. If you're building a SDK consumer for your own use, requiring more steps (or, pragmatically, probably just a larger copy-paste) seems onerous. If you're using an SDK consumer that someone else built... well they could easy write the code to consume CLI config files for authentication.

The GCP model you describe sounds like more of a pain in the neck, but if you think we should go down that path let's discuss. I'd note that the SDK today basically pulls data out of the hosts.toml file.

augustuswm · 2024-07-15T17:48:47Z

Initial run through of basic commands with the CLI seemed to work (mostly just reads). Generally worked as expected. Nicely handled having two different silos with the same name Two minor suggestions:

[oxide.rs] new-config : oxide system networking bgp status
no authenticated hosts; use oxide auth login to authenticate

Maybe a newline here between the statements? or something else to make oxide auth login stand out.

Was there a specific reason to make the profile flag non-global? I initially placed it at the end of the call:

[oxide.rs] new-config : oxide system networking bgp show-status --profile oxide2
error: unexpected argument '--profile' found

Usage: oxide system networking bgp show-status

For more information, try '--help'.

expecting behavior like the AWS CLI. Given that I know the behavior of clap it was easy enough to realize that I should move it to the front, though that may not be common knowledge.

ahl · 2024-07-15T18:27:40Z

@augustuswm

Was there a specific reason to make the profile flag non-global? I initially placed it at the end of the call:

I actually tinkered with that, but the docs put --profile under every subcommand which I thought was distracting.

david-crespo · 2024-07-15T18:41:42Z

We should be able to fix that in the docs somehow. I think the UX benefit of being able to put it anywhere is important. I would be absolutely baffled by that error. Think we could get a global property into the docs JSON? Then we could decide in the docs to either hide it or give its own section at the bottom. Could add that in #725.

Edit: https://docs.rs/clap/latest/clap/struct.Arg.html#method.is_global_set

david-crespo · 2024-07-15T19:09:08Z

cli/README.md

+[here](https://github.com/oxidecomputer/oxide.rs/releases). Look for the most
+recent release whose version suffix matches the version of Oxide software
+currently deployed. For example, if you're running the `20240502.0` release,
+use `0.5.0+20240502.0`.


There might not be anything to change here about this, but 20240502.0 is the API version but "v8" is the primary release version. Travis just asked me the other day whether there was a way to know in the console what release you're on. Not only is there no way to do that, but even if you did know you were on v8, there isn't a way to know that that corresponds to 20240502.0, or vice versa. It's worth noting that there is a note in the v8 release notes about 0.5.0 being the relevant CLI version.

Like I said, probably a bigger problem we can't fix in the readme for the CLI, but this reminded me of the problem — something to mull over as we keep tweaking this stuff.

My hope is that our docs would send you to the right CLI version and that the CLI itself will be able to point you to the right version based on a response from the server. We're going to have the generated client send the version as a header. The server response can include an indication if you're on a mismatched version (ie with work in dropshot / omicron)

david-crespo

I concede

ahl · 2024-07-15T22:44:04Z

If I missed anything from the review above, please file as an issue and I'll attend to it.

ahl added 4 commits July 3, 2024 17:31

big chunk working; need to reorganize the configuration stuff

3163e58

good progress on all subcommands. todo:

77b19da

- rip out old config stuff - only create the Client when we need it

ripped out the old world; seems nicer. Next:

bd0ace0

- test the auth commands under various conditions

clean up for CI

61fa486

ahl requested a review from david-crespo July 8, 2024 16:54

provide a raw token auth method in the SDK

502d70a

karencfv reviewed Jul 8, 2024

View reviewed changes

not global

bd96e4c

updates

3d7cd25

ahl added 3 commits July 12, 2024 16:45

tested and in pretty good shape

ba8d7ba

Merge branch 'main' into new-config

8e58b1d

merge with main

55a02f7

ahl changed the title ~~WIP: re-organize CLI config files~~ re-organize CLI config files Jul 12, 2024

ahl added 2 commits July 12, 2024 17:01

readme cleanup

1d20d93

normalize dns error output

c6f2632

ahl commented Jul 13, 2024

View reviewed changes

self-review

9735b9a

ahl marked this pull request as ready for review July 13, 2024 00:25

windows 😡

d0bfb0c

karencfv reviewed Jul 15, 2024

View reviewed changes

david-crespo reviewed Jul 15, 2024

View reviewed changes

Merge branch 'main' into new-config

0594adc

david-crespo approved these changes Jul 15, 2024

View reviewed changes

ahl merged commit 32890d3 into main Jul 15, 2024
16 checks passed

ahl deleted the new-config branch July 15, 2024 22:44

wfchandler mentioned this pull request Jul 16, 2024

CLI: print error source #740

Merged

david-crespo mentioned this pull request Aug 8, 2024

Accept profile argument in all subcommands #781

Merged

karencfv mentioned this pull request Aug 12, 2024

Authorization via oxide profiles oxidecomputer/terraform-provider-oxide#334

Open

		// TODO what's the point of this?
		let client_id = Uuid::new_v4();

re-organize CLI config files #727

re-organize CLI config files #727

Conversation

ahl commented Jul 8, 2024 • edited Loading

david-crespo commented Jul 8, 2024 • edited Loading

ahl commented Jul 8, 2024

karencfv left a comment

Choose a reason for hiding this comment

karencfv Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahl commented Jul 9, 2024

augustuswm commented Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

ahl commented Jul 13, 2024

ahl commented Jul 13, 2024

david-crespo commented Jul 13, 2024 • edited Loading

ahl commented Jul 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahl Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

augustuswm commented Jul 15, 2024

ahl commented Jul 15, 2024

augustuswm commented Jul 15, 2024

ahl commented Jul 15, 2024

david-crespo commented Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-crespo left a comment

Choose a reason for hiding this comment

ahl commented Jul 15, 2024

ahl commented Jul 8, 2024 •

edited

Loading

david-crespo commented Jul 8, 2024 •

edited

Loading

karencfv Jul 8, 2024 •

edited

Loading

augustuswm commented Jul 12, 2024 •

edited

Loading

david-crespo commented Jul 13, 2024 •

edited

Loading

ahl Jul 15, 2024 •

edited

Loading

david-crespo commented Jul 15, 2024 •

edited

Loading