Skip to content

Take paths instead of base64 for SAML creation#1112

Merged
wfchandler merged 4 commits into
mainfrom
wc/saml-sanity
May 28, 2025
Merged

Take paths instead of base64 for SAML creation#1112
wfchandler merged 4 commits into
mainfrom
wc/saml-sanity

Conversation

@wfchandler
Copy link
Copy Markdown
Collaborator

@wfchandler wfchandler commented May 23, 2025

Currently the CLI's SAML IdP creation flow does not provide flags for signing keys, requiring the cryptic --json-body argument. These and the XML metadata must be manually base64-encoded by the user as well.

Make this process more convenient by converting the keys and XML metadata args to take the path of the source files, which we then read and encode ourselves.

While we're at it, stop generating docs for hidden items.

Example command:

$ oxide silo idp saml create \
--silo cli \
--name "keycloak" \
--description "Keybloke" \
--sp-client-id cli \
--acs-url "https://cli.sys.example.com/login/cli/saml/keycloak" \
--idp-entity-id "https://keycloak.example.com/realms/cli" \
--slo-url "" \
--technical-contact-email "user@example.com" \
--group-attribute-name group
--metadata-path ./cli-metadata.xml \
--public-cert-path ./cli-cert.der \
--private-key-path ./cli-key.der

Currently the CLI's SAML IdP creation flow does not provide flags for
signing keys, requiring the cryptic `--json-body` argument. These and
the XML metadata must be manually base64-encoded by the user as well.

Make this process more convenient by converting the keys and XML
metadata args to take the path of the source files, which we then read
and encode ourselves.

While we're at it, hide the no longer needed `--json-body` and
`--json-body-template` arguments in the CLI and stop generating docs
for hidden items.

Example command:

```
$ oxide silo idp saml create \
--silo cli \
--name "keycloak" \
--description "Keybloke" \
--sp-client-id cli \
--acs-url "https://cli.sys.example.com/login/cli/saml/keycloak" \
--idp-entity-id "https://keycloak.example.com/realms/cli" \
--slo-url "" \
--technical-contact-email "user@example.com" \
--group-attribute-name group
--metadata-path ./cli-metadata.xml \
--public-cert-path ./cli-cert.der \
--private-key-path ./cli-key.der \
```
@wfchandler wfchandler requested a review from ahl May 23, 2025 18:39
Copy link
Copy Markdown
Collaborator

@ahl ahl left a comment

Choose a reason for hiding this comment

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

test?

Comment thread cli/src/cli_builder.rs Outdated
Comment on lines +138 to +139
.mut_arg("json-body", |arg| arg.required(false).hide(true))
.mut_arg("json-body-template", |arg| arg.hide(true))
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We show these for all commands that take bodies, even if they're not required. What's the motivation here?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I find json-body confusing, personally, and I had the impression we wanted to move away from it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We want to move away from requiring it; I don't think we want to move away from supporting it. What do you find confusing?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Ah OK, my mistake. Mostly it's not obvious when --json-body is required, and how it composes with the rest of the command. If I set a value in JSON and as an argument, which one wins? Are there fields that can only be set via JSON?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's write those things down

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Where? Ideally it ends up in the error message when you use them incorrectly. Maybe even in the non-error help output, if it's not too long.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I've opened #1119 to track this.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you!!

Comment thread cli/src/cli_builder.rs Outdated
Comment thread cli/src/cli_builder.rs Outdated
@wfchandler wfchandler requested a review from ahl May 27, 2025 16:38
Copy link
Copy Markdown
Collaborator

@ahl ahl 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 adding the test!

Comment thread cli/src/cmd_docs.rs
let mut args = cmd
.get_arguments()
.filter(|arg| arg.get_long() != Some("help"))
.filter(|arg| !arg.is_hide_set())
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this is fine, but is it still relevant?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think it's worth leaving in to stop documenting the --parallelism argument

.arg("http://nope.nope")
.arg("--technical-contact-email")
.arg("anyone@but.adam")
.arg("--private-key")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this fails because this guy didn't have his friend?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yup, that's correct. Do you want a comment here to be explicit?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm thinking of myself in the future, breaking this, but not noticing.

@wfchandler wfchandler merged commit 11fe44b into main May 28, 2025
16 checks passed
@wfchandler wfchandler deleted the wc/saml-sanity branch May 28, 2025 15:40
wfchandler added a commit that referenced this pull request Nov 12, 2025
Customers have found it inconvenient and error-prone to pass in the
contents of certs and keys to the `certificate create` subcommand. To
make this command easier to use, update its `key` and `cert` arguments
to be treated as the path to their respective files, rather than their
contents.

We previously updated the SAML IdP creation command the same way with
11fe44b (Take paths instead of base64 for SAML creation (#1112),
2025-05-28).
wfchandler added a commit that referenced this pull request Nov 14, 2025
Customers have found it inconvenient and error-prone to pass in the
contents of certs and keys to the `certificate create` subcommand. To
make this command easier to use, update its `key` and `cert` arguments
to be treated as the path to their respective files, rather than their
contents.

We previously updated the SAML IdP creation command the same way with
11fe44b (Take paths instead of base64 for SAML creation (#1112),
2025-05-28).

Co-authored-by: Adam Leventhal <ahl@oxide.computer>
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.

3 participants