Support other nextstrain.org-like remotes #333

tsibley · 2023-11-22T01:03:01Z

Adds support for other nextstrain.org-like remotes to the nextstrain remote family of commands, along with support for OIDC/OAuth2 authentication with them to the nextstrain login and related commands. Required IdP and client configuration for OIDC/OAuth2 is auto-discovered.

One giant pile of changes, because I did not have time to go thru my typical editing and splitting process before going on leave. :/ Ah well.

Related-to: https://github.com/nextstrain/private/issues/94

Builds upon #332.

@victorlin I'm hopeful you can pick this up if needed before I'm back. I wish I'd had time to break it up into smaller changes.

At the least, without any additional work, this should provide CI builds of the standalone archives for testing, e.g. by installing them with:

curl -fsSL --proto '=https' https://nextstrain.org/cli/installer/linux | bash -s pr-build/333

To test this against our Azure AD/Entra ID testing directory, I created an app registration for the CLI (68e51465-2672-4c47-92ab-bec0e919cf7a). This required mapping AD/security groups to app roles, just like was done for the web server client. I then ran the nextstrain.org codebase with this bit of config.

{
  "AWS_REGION": "us-east-1",
  "OIDC_IDP_URL": "https://login.microsoftonline.com/0ce9e8dc-e009-4cb4-8512-7989bd6906a8/v2.0",
  "OAUTH2_CLIENT_ID": "c3d6647f-dccc-4a85-a2bb-fb8fbc7524a9",
  "OAUTH2_CLI_CLIENT_ID": "68e51465-2672-4c47-92ab-bec0e919cf7a",
  "OAUTH2_CLI_CLIENT_REDIRECT_URIS": ["http://127.0.0.1"],
  "OIDC_USERNAME_CLAIM": "preferred_username",
  "OIDC_GROUPS_CLAIM": "roles",
  "OIDC_IAT_BACKDATED_BY": 300,
  "GROUPS_DATA_FILE": "groups.json"
}

Note the redirect URI! This is the appropriate URI for production too, not just testing. A secret is also required, which is configured by env var.

Testing looks like then:

nextstrain login localhost:5000  # the local nextstrain.org server, as configured above
nextstrain whoami localhost:5000
nextstrain remote ls localhost:5000/groups/test-private/

Checklist

Checks pass
Type checking failures need to be sorted out
Further testing internally and by CDC
Later: document remotes and authn flow (Remotes doc #334)

victorlin

Forgot to mention that I made a few patches while looking into this for usage by CDC. Consider them as suggestions for this PR: 39fdb8a...ea3d185

tsibley · 2024-01-12T19:16:13Z

Great, thanks!

tsibley · 2024-01-12T19:18:24Z

Brought all three into my local branch with:

git am < <(curl -fsSL https://github.com/nextstrain/cli/compare/39fdb8a44334f6d8fce67a958c534bc7d880a9d2...ea3d1851becf1ab2b6bf1257c2338525414190f9.patch)

and will probably fix them up into the primary commit.

These come with the standard typing library as of Python 3.8.¹ ¹ <https://docs.python.org/3.8/whatsnew/3.8.html#typing>

Mypy 1.8.0 and some earlier versions complain about these two imports: $ mypy -p nextstrain.cli nextstrain/cli/runner/singularity.py:98: error: Name "docker" already defined (by an import) [no-redef] nextstrain/cli/runner/aws_batch/__init__.py:94: error: Name "docker" already defined (by an import) [no-redef] Found 2 errors in 2 files (checked 61 source files) …but there is no other definition of "docker" in those files! Mypy's got something wrong. These seem like new complaints as the code has existed for a while. Pyright does not complain. Adding reveal_type(docker) before the imports to see what each thinks produces: note: Revealed type is "types.ModuleType" from Mypy and information: Type of "docker" is "Unbound" from Pyright.

Adds support for other nextstrain.org-like remotes to the `nextstrain remote` family of commands, along with support for OIDC/OAuth2 authentication with them to the `nextstrain login` and related commands. Required IdP and client configuration for OIDC/OAuth2 is auto-discovered. One giant pile of changes, because I did not have time to go thru my typical editing and splitting process before going on leave. :/ Ah well. Related-to: <nextstrain/private#94>

It's handy and reassuring to be able to clear all login credentials without remembering what remotes you're logged into.

victorlin · 2024-01-17T20:01:21Z

Steps to test this for current Nextstrain users:

# Replace 'mac' with 'linux' or 'windows' if necessary
curl -fsSL --proto '=https' https://nextstrain.org/cli/installer/mac | bash -s pr-build/333

# Check you have the expected version
nextstrain version
# 7.4.0+git.aa9795f

nextstrain logout

nextstrain login
# Follow prompts

huddlej · 2024-01-17T20:18:23Z

@victorlin Those commands all ran as expected. I was able to remote ls and remote download with a private group after logging in.

huddlej · 2024-01-17T22:15:30Z

@tsibley Confirmed that I wasn't testing the right thing because I installed the new CLI version and then ran a different older version that happened to have priority in my PATH. After confirming I was using the correct version, I got the web-based login and authentication worked as expected. I was surprised by the text output on the terminal, however, that said:

URL: (nevermind!)

corneliusroemer · 2024-01-17T22:38:52Z

Tested web login in an Ubuntu docker container and it worked with manual copying of URL

jameshadfield

Looks good by (brief) inspection. Minor comments. Tested via an otherwise empty docker image using the default nextstrain.org remote.

Login flow via OIDC worked well, required manual pasting of URLs into the browser and then back to the CLI.
Login flow using --username worked well.

jameshadfield · 2024-01-18T00:56:18Z

CHANGES.md

+  `--username` or `-u`).  See `nextstrain login --help` for more information.
+  ([#333](https://github.com/nextstrain/cli/pull/333))
+
+* With the new support for being logged into multiple remotes, `nextstrain


What's the use case for being logged into multiple remotes simultaneously?

CDC will be running a Nextstrain Groups Server internally but also has Groups on nextstrain.org they use, so they'll be accessing two remotes: nextstrain.org and nextstrain.biotech.cdc.gov.

And other organizations have also expressed interest in having a Groups server they can use internally while still maintaining public Groups on nextstrain.org.

For development, it's also useful to be able to be manipulate authn with localhost:5000 without disrupting your normal nextstrain.org authn status.

jameshadfield · 2024-01-18T01:03:23Z

doc/development.md


    nextstrain login
    nextstrain whoami
    nextstrain remote ls groups/test-private

+Most of the times the above is not necessary, however, as you can specify the
+local remote explicitly instead of pretending it's nextstrain.org, e.g.:


I interpret this section as saying the following are equivalent:

Set default remote via env variable, e.g. NEXTSTRAIN_DOT_ORG=http://localhost:5000 nextstrain login and NEXTSTRAIN_DOT_ORG=http://localhost:5000 nextstrain remote ls groups/test.

Specify remote in command, e.g. nextstrain login localhost:5000, nextstrain remote ls http://localhost:5000/groups/test

And that neither will access https://nextstrain.org. Is this correct?

I have only a cursory understanding of how the CLI, nextstrain.org server (perhaps localhost) and cognito are interacting here.

When we login with nextstrain login localhost:5000 with the nextstrain.org server running at localhost:5000, is this remote the authz server as far as the CLI is concerned? And thus everything to do with cognito (user pools etc) has been lifted out of the CLI and into the remote? And thus any tokens the CLI receives are only ever interacting with the provided remote?

I interpret this section as saying the following are equivalent: […] And that neither will access https://nextstrain.org/. Is this correct?

They're equivalent except for the short code path before NEXTSTRAIN_DOT_ORG is used to potentially override what "nextstrain.org" means (in nextstrain.cli.remote.parse_remote_path()). That is, once the remote is parsed, they're equivalent.

Neither will access https://nextstrain.org itself.

When we login with nextstrain login localhost:5000 with the nextstrain.org server running at localhost:5000, is this remote the authz server as far as the CLI is concerned? And thus everything to do with cognito (user pools etc) has been lifted out of the CLI and into the remote? And thus any tokens the CLI receives are only ever interacting with the provided remote?

No, the CLI still interacts with the OIDC/OAuth2 authorization server (aka the IdP), which in our case is Cognito but in CDC's case is Entra ID (née Azure AD). It obtains the IdP information from the remote, so it doesn't have to be hardcoded. I do have a rough WIP of docs for this interaction to help folks understand, but it's not up yet.

Ok. At least this way the identity provider configuration has been lifted out of the CLI and into the remote.

jameshadfield · 2024-01-18T01:07:47Z

nextstrain/cli/command/login.py

-    print(f"Logged into nextstrain.org as {user.username}.")
-    print("Log out with `nextstrain logout`.")
+    print(f"Logged into {url.origin} as {user.username}.")
+    print(f"Log out with `nextstrain logout {shquote(url.origin)}`.")


I would favor not printing the remote when it's the default (nextstrain.org).

Hmm, I don't agree. I think because multiple remotes are possible, it's good to always be explicit to avoid confusing output.

tsibley · 2024-01-18T20:05:59Z

Overall feedback seems positive and CDC is already running (successfully) the development builds from this PR, so I'll plan to merge and release today.

I'd definitely welcome a deeper review of code and functionality and would be happy to discuss post-merge suggestions.

victorlin

Lots to digest in b4b29d0 but it was understandable. Looks good 👍

nextstrain/cli/authn/configuration.py

doc/commands/whoami.rst

nextstrain/cli/authn/__init__.py

nextstrain/cli/authn/session.py

@victorlin

@victorlin pointed out¹ that a UserError is not raised by this function. While correcting that, I noted that other docstrings in this module use "saved credentials" as the term (matching the term we use in user-visible output) instead of "tokens", so I made this docstring follow suit. ¹ <#333 (comment)>

@victorlin

@victorlin suggested¹ we improve error handling in this code path, and he's right. Catch and improve the output for the three most common errors we'll encounter: connection errors, HTTP errors, and JSON decoding errors. The original error is summarized in the error output, and by chaining the new exception from the original, the full chain in all its detail will be printed when NEXTSTRAIN_DEBUG=1 for troubleshooting. ¹ <#333 (comment)>

victorlin mentioned this pull request Nov 28, 2023

CLI installer doesn't work for failed PR builds that still have artifacts nextstrain/nextstrain.org#759

Closed

victorlin mentioned this pull request Dec 15, 2023

Unhandled exception when renewing login from a different Cognito user pool #339

Open

tsibley mentioned this pull request Jan 11, 2024

Various and sundry #332

Merged

1 task

Base automatically changed from trs/various-and-sundry to master January 11, 2024 20:16

victorlin reviewed Jan 12, 2024

View reviewed changes

tsibley added 2 commits January 12, 2024 12:37

dev: Remove use of typing_extensions.Literal and .Protocol

a1cc22d

These come with the standard typing library as of Python 3.8.¹ ¹ <https://docs.python.org/3.8/whatsnew/3.8.html#typing>

tsibley force-pushed the trs/remotes branch 2 times, most recently from 288be61 to e74ee80 Compare January 12, 2024 23:01

tsibley added 2 commits January 12, 2024 17:15

logout: Add --all flag remove all locally-saved credentials

aa9795f

It's handy and reassuring to be able to clear all login credentials without remembering what remotes you're logged into.

tsibley force-pushed the trs/remotes branch from a229ead to aa9795f Compare January 13, 2024 01:16

tsibley marked this pull request as ready for review January 13, 2024 01:16

tsibley requested a review from a team January 13, 2024 01:16

tsibley mentioned this pull request Jan 18, 2024

Supporting downloading datasets not in manifest #345

Merged

1 task

jameshadfield reviewed Jan 18, 2024

View reviewed changes

tsibley merged commit cc0036b into master Jan 18, 2024
36 checks passed

tsibley deleted the trs/remotes branch January 18, 2024 20:10

victorlin reviewed Jan 26, 2024

View reviewed changes

nextstrain/cli/authn/configuration.py Show resolved Hide resolved

doc/commands/whoami.rst Show resolved Hide resolved

nextstrain/cli/authn/__init__.py Show resolved Hide resolved

nextstrain/cli/authn/session.py Show resolved Hide resolved

tsibley mentioned this pull request Jan 27, 2024

Review feedback: Support other nextstrain.org-like remotes #347

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support other nextstrain.org-like remotes #333

Support other nextstrain.org-like remotes #333

tsibley commented Nov 22, 2023 •

edited

Loading

victorlin left a comment

tsibley commented Jan 12, 2024

tsibley commented Jan 12, 2024

victorlin commented Jan 17, 2024 •

edited by tsibley

Loading

huddlej commented Jan 17, 2024

huddlej commented Jan 17, 2024

corneliusroemer commented Jan 17, 2024

jameshadfield left a comment

jameshadfield Jan 18, 2024

tsibley Jan 18, 2024

tsibley Jan 18, 2024

jameshadfield Jan 18, 2024 •

edited

Loading

jameshadfield Jan 18, 2024 •

edited

Loading

tsibley Jan 18, 2024

jameshadfield Jan 18, 2024

jameshadfield Jan 18, 2024

tsibley Jan 18, 2024

tsibley commented Jan 18, 2024

victorlin left a comment

Support other nextstrain.org-like remotes #333

Support other nextstrain.org-like remotes #333

Conversation

tsibley commented Nov 22, 2023 • edited Loading

Checklist

victorlin left a comment

Choose a reason for hiding this comment

tsibley commented Jan 12, 2024

tsibley commented Jan 12, 2024

victorlin commented Jan 17, 2024 • edited by tsibley Loading

huddlej commented Jan 17, 2024

huddlej commented Jan 17, 2024

corneliusroemer commented Jan 17, 2024

jameshadfield left a comment

Choose a reason for hiding this comment

jameshadfield Jan 18, 2024

Choose a reason for hiding this comment

tsibley Jan 18, 2024

Choose a reason for hiding this comment

tsibley Jan 18, 2024

Choose a reason for hiding this comment

jameshadfield Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

jameshadfield Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

tsibley Jan 18, 2024

Choose a reason for hiding this comment

jameshadfield Jan 18, 2024

Choose a reason for hiding this comment

jameshadfield Jan 18, 2024

Choose a reason for hiding this comment

tsibley Jan 18, 2024

Choose a reason for hiding this comment

tsibley commented Jan 18, 2024

victorlin left a comment

Choose a reason for hiding this comment

tsibley commented Nov 22, 2023 •

edited

Loading

victorlin commented Jan 17, 2024 •

edited by tsibley

Loading

jameshadfield Jan 18, 2024 •

edited

Loading

jameshadfield Jan 18, 2024 •

edited

Loading