Support for SAML as a Silo IdP, part 1 #994

jmpesp · 2022-04-29T13:56:48Z

Add the db schemas, models, and some endpoints to support configuring a
SAML IdP for a Silo. Enough functionality is here to support the first
step of SP-initiated SAML login flow. More work is required to support
receiving the SAML IdP's response, and actually creating and logging in
the user.

Two tables were added here: one that relates a silo to a list of typed
identity providers, and one for saml configuration. Future work will add
"local" and "ldap" identity provider support.

Table column order was corrected - I didn't see any bugs from this but
am aware that the bug potential exists.

Currently, the external-authenticator role only applies to the
default silo, not any new ones. Making that opctx work with any silo
will unlock some more testing.

This commit also moves the unauthorized_coverage code into the
unauthorized function, and sets up both hardcoded and new non-hardcoded
data. When global images support was added, a hardcoded port was used
for the httptest server and it was a problem to find an unused port, but
allowing for non-hardcoded data means any port can be used.

Add the db schemas, models, and some endpoints to support configuring a SAML IdP for a Silo. Enough functionality is here to support the first step of SP-initiated SAML login flow. More work is required to support receiving the SAML IdP's response, and actually creating and logging in the user. Two tables were added here: one that relates a silo to a list of typed identity providers, and one for saml configuration. Future work will add "local" and "ldap" identity provider support. Table column order was corrected - I didn't see any bugs from this but am aware that the bug potential exists. Currently, the external-authenticator role only applies to the default silo, not any new ones. Making that opctx work with any silo will unlock some more testing. This commit also moves the unauthorized_coverage code into the unauthorized function, and sets up both hardcoded and new non-hardcoded data. When global images support was added, a hardcoded port was used for the httptest server and it was a problem to find an unused port, but allowing for non-hardcoded data means any port can be used.

jmpesp · 2022-04-29T14:09:26Z

Looks like everything's failing because xmlsec1 isn't installed. Looking into it.

nexus/src/db/model.rs

david-crespo · 2022-04-29T16:27:12Z

nexus/src/db/model.rs

+                    )*
+                }
+            }
+        }


davepacheco · 2022-04-29T18:13:00Z

Is xmlsec1 something that's now expected to be installed on the build/deployment machine and we depend on it at build and run time (similar to libpq, etc.)? In that case, I think you want to update the install_prerequisites script and not add new steps to the GitHub/buildomat workflows. I say that because those workflows won't be run on individuals' machines, whereas the install_prerequisites script is intended to be run by devs and it's also run in CI so it should cover all cases and actually gets tested regularly.

david-crespo · 2022-04-29T18:26:39Z

We couldn't confirm that install_prerequisites.sh was run on CI, where is that?

davepacheco · 2022-04-29T18:57:37Z

Dang, I had thought #870 already landed. I think maybe we want to land that?

jmpesp · 2022-04-29T19:51:32Z

Unfortunately, there's also no xmlsec1 packages for helios.

EDIT: or OmniOS

jmpesp · 2022-05-02T14:41:56Z

Unfortunately, there's also no xmlsec1 packages for helios.

xmlsec1 does build on helios. the following completes ok:

git clone https://github.com/lsh123/xmlsec
cd xmlsec/
pfexec pkg install pkg:/developer/build/libtool pkg:/library/libtool/libltdl
./autogen.sh
gmake -j $(nproc)

Paging @jclulow - how do packages get built for helios? Is this something I can do?

jclulow · 2022-05-03T11:54:34Z

Unfortunately, there's also no xmlsec1 packages for helios.
Paging @jclulow - how do packages get built for helios? Is this something I can do?

It's still a bit manual, so I've gone ahead and written the recipe and published the package for now. In the future, yes, I want things to be more self-service!

I took version 1.2.33 (latest stable) and built it into this package:

pkg://helios-dev/library/[email protected]:20220503T112921Z

You should be able to install that to get the libraries and the xmlsec(1) binary etc. The build recipe is in the garbage compactor.

jmpesp · 2022-05-03T18:05:49Z

@jclulow thanks for turning this around so quickly - I opened a PR in the garbage-compactor repo to address a dynamic loading problem with xmlsec, but once that's in it builds and runs on helios no problem.

@smklein what do you think about landing #870?

smklein · 2022-05-04T15:00:44Z

@jclulow thanks for turning this around so quickly - I opened a PR in the garbage-compactor repo to address a dynamic loading problem with xmlsec, but once that's in it builds and runs on helios no problem.

@smklein what do you think about landing #870?

I just rebased #870 - should be ready to go now.

remove silo_id from params, it was not used

add DerEncodedKeyPair type to add a bit of API safety remove get prefix from functions validate DER keys

Use serde's deserialize_with to validate public cert and private keys, causing failure if the DerEncodedKeyPair isn't valid. The visitor used here is a fn from `String -> Result<String>` which allows for early validation, and afterwards any code operating on DerEncodedKeyPair can be sure it contains valid data. Unfortunately, the deserialize_with could not be a fn from `String -> an openssl::type` because the openssl types do not derive JsonSchema. Previously some of this validation was in SiloSamlIdentityProvider's validate, and that has been removed in this commit.

Instead of putting an impl for the model::SiloSamlIdentityProvider type into the authn subsystem, make a new authn type, and convert the model type into that. Move the logic that looks up SiloIdentityProviderType from nexus.rs to authn silo subsystem. Move validate function logic into a new TryFrom impl for the authn SiloSamlIdentityProvider type. This way it's part of the conversion and cannot be forgotten about.

Ran find */ -type f -exec sed -i -e 's/SiloIdentityProvider/IdentityProvider/g' {} + ; find */ -type f -exec sed -i -e 's/SILO_IDENTITY_PROVIDER/IDENTITY_PROVIDER/g' {} + ; find */ -type f -exec sed -i -e 's/silo_identity_provider/identity_provider/g' {} + ; find */ -type f -exec sed -i -e 's/SiloSamlIdentityProvider/SamlIdentityProvider/g' {} + ; find */ -type f -exec sed -i -e 's/SILO_SAML_IDENTITY_PROVIDER/SAML_IDENTITY_PROVIDER/g' {} + ; find */ -type f -exec sed -i -e 's/silo_saml_identity_provider/saml_identity_provider/g' {} + ; and git mv ./nexus/src/db/model/silo_identity_provider.rs ./nexus/src/db/model/identity_provider.rs

davepacheco · 2022-05-12T23:43:38Z

nexus/src/authn/silos.rs

+                            // model to the authn type here, this is a server
+                            // error: it was validated before it went into the
+                            // DB.
+                            omicron_common::api::external::Error::internal_error(&e.to_string())


Can you add a tiny bit of context here (like maybe make that argument &format!("deserializing SAML IdP from database: {:#}", e)? I'm afraid if we ever hit this case it'll be very hard to tell what we were even trying to parse.

Definitely, context added in 46de770

davepacheco · 2022-05-12T23:58:06Z

nexus/src/db/lookup.rs

@@ -397,7 +397,7 @@ impl<'a> Root<'a> {
 lookup_resource! {
    name = "Silo",
    ancestors = [],
-    children = [ "Organization" ],
+    children = [ "Organization", "SiloIdentityProvider", "SiloSamlIdentityProvider" ],


Okay. I think "backend" is consistent with its use in other distributed systems contexts (e.g., "backend task" in this SRE book chapter), but I'm definitely not attached to it!

jmpesp · 2022-05-25T14:14:52Z

There's too much going on in this PR, plus a lot has changed in main, PLUS even if I do rebase the whole thing it will remain blocked on Josh's PR upstreaming the "dynamic" branch of our samael fork. I'm going to close this, and break it up into several smaller PRs so the parts unrelated to the main SAML functionality can go in.

david-crespo reviewed Apr 29, 2022

View reviewed changes

nexus/src/db/model.rs Outdated Show resolved Hide resolved

david-crespo reviewed Apr 29, 2022

View reviewed changes

nexus/src/db/model.rs Outdated Show resolved Hide resolved

use impl_enum_type for SiloIdentityProviderType

81dd77f

david-crespo reviewed Apr 29, 2022

View reviewed changes

nexus/src/db/model.rs Outdated Show resolved Hide resolved

jmpesp added 3 commits April 29, 2022 11:29

install libxmlsec1-openssl in Dockerfile

13b091d

impl ToString in the impl_enum_type macro

1e87918

xmlsec1 too

6be4f9f

david-crespo reviewed Apr 29, 2022

View reviewed changes

nexus/src/db/model.rs Outdated

)*

}

}

}

Copy link

Contributor

david-crespo Apr 29, 2022

Choose a reason for hiding this comment

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

👌

try installing xmlsec1 in check-omicron-deployment

ed33499

put package installs into install_prerequisites.sh

f55fada

Merge remote-tracking branch 'upstream/main' into silo_authn_providers

ffc9f96

no more full table scans

7595933

jmpesp added 8 commits May 4, 2022 16:13

Merge remote-tracking branch 'upstream/main' into silo_authn_providers

2566741

install library/libxmlsec1 on helios

b578a99

update silo identity provider diesel code

463b1b6

./tools/install_prerequisites.sh for clippy-lint

99bcf08

update PATH

a12939b

install libxmlsec1-dev for pkg-config files

3ec5827

Merge remote-tracking branch 'upstream/main' into silo_authn_providers

3eb6e98

fmt

115ffdd

jmpesp added 15 commits May 6, 2022 13:54

ErrorHandler::NotFoundByResource

8a6f954

change to String::from_utf8_lossy

b083fe5

remove local and ldap from dbinit

8571681

fmt

1ed412e

turns out I can add id without test failure...?

74521e8

use ErrorHandler::Conflict

813876a

better comment

56320f3

use lazy_static http server, restore unauthorized_coverage

6597d67

remove silo_id from params, it was not used

properly timeout, and return better error messages for idp urls

c9d369c

saml_identity_provider -> saml_identity_providers

36d3588

add view for SiloSamlIdentityProvider

5f352d0

add DerEncodedKeyPair type to add a bit of API safety remove get prefix from functions validate DER keys

clippy and fmt

9593b8f

davepacheco reviewed May 12, 2022

View reviewed changes

jmpesp added 9 commits May 13, 2022 16:32

add error context with try_into fails

46de770

typo

6c42a7c

add regular identity to identity provider

3393799

add support for listing identity providers

2f45e1a

larger request_body_max_bytes for SAML IdP payload

fc0522d

get by with samael patch and "cargo update -p clang-sys" for now

302f878

uncomment test_listing_identity_providers

140f697

add list_identity_providers permission

f9e2011

use lazy_static for saml identity provider stuff

609a7e1

jmpesp closed this May 25, 2022

jmpesp deleted the silo_authn_providers branch May 25, 2022 14:15

This was referenced May 25, 2022

remove copied comment on USER_EXTERNAL_AUTHN #1116

Merged

use lazy_static for httptest server #1118

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for SAML as a Silo IdP, part 1 #994

Support for SAML as a Silo IdP, part 1 #994

jmpesp commented Apr 29, 2022

jmpesp commented Apr 29, 2022

david-crespo Apr 29, 2022

davepacheco commented Apr 29, 2022

david-crespo commented Apr 29, 2022

davepacheco commented Apr 29, 2022

jmpesp commented Apr 29, 2022 •

edited

Loading

jmpesp commented May 2, 2022

jclulow commented May 3, 2022

jmpesp commented May 3, 2022

smklein commented May 4, 2022

davepacheco May 12, 2022

jmpesp May 13, 2022

davepacheco May 12, 2022

jmpesp commented May 25, 2022

Support for SAML as a Silo IdP, part 1 #994

Support for SAML as a Silo IdP, part 1 #994

Conversation

jmpesp commented Apr 29, 2022

jmpesp commented Apr 29, 2022

david-crespo Apr 29, 2022

Choose a reason for hiding this comment

davepacheco commented Apr 29, 2022

david-crespo commented Apr 29, 2022

davepacheco commented Apr 29, 2022

jmpesp commented Apr 29, 2022 • edited Loading

jmpesp commented May 2, 2022

jclulow commented May 3, 2022

jmpesp commented May 3, 2022

smklein commented May 4, 2022

davepacheco May 12, 2022

Choose a reason for hiding this comment

jmpesp May 13, 2022

Choose a reason for hiding this comment

davepacheco May 12, 2022

Choose a reason for hiding this comment

jmpesp commented May 25, 2022

jmpesp commented Apr 29, 2022 •

edited

Loading