Oidc autoregistration #152

brutif · 2020-05-08T19:27:11Z

What this PR does / why we need it?:
Adds ability for oidc sso clients to automatically register with RedHat Single Sign-On
Does this PR introduce a user-facing change? yes

User guide
CHANGELOG.md

Which issue(s) this PR fixes:
Addresses kabanero-io/kabanero-security#71

arthurdm · 2020-05-27T13:44:34Z

doc/user-guide.adoc

+  #
+  # Optional: To skip TLS certificate checking with the provider, specify insecureTLS as true.  Default is false.
+  # insecureTLS: dHJ1ZQ==
+----


can we also add what the corresponding RuntimeComponent CR would be, and how it references this Secret? It would be good to visualize a working sample of what fields are needed in this auto-registration flow. Is there a specific naming convention that one must follow for this Secret to be auto discovered, or it can be named whatever we want as long as we reference it from the CR?

The secret is an attribute of the OpenLibertyApplication, sso.oidc[].autoRegisterSecret, so it's not usable from the RuntimeComponent CR. It can be named whatever we want. I've updated the user guide to make the flow and who needs what (RedHat SSO vs. IBM Cloud Identity) more clear.

my bad - I meant to say the corresponding OpenLibertyApplication CR sample, that shows a complete CR using autoRegisterSecret. I really like the Secret example, I think we just need the corresponding olapp CR.

ah, got it. Added an example.

arthurdm · 2020-06-01T15:35:26Z

thanks @brutif - the doc / high level changes looks good to me. I will defer to @leochr for the code changes review.

leochr

@brutif Added some initial comments. Yet to review some code. Thanks.

leochr · 2020-05-21T16:46:37Z

pkg/controller/openliberty/openlibertyapplication_controller.go

-				secretName := instance.GetName() + ssoSecretNameSuffix
-				ssoSecret := &corev1.Secret{}
-				err := r.GetClient().Get(context.TODO(), types.NamespacedName{Name: secretName, Namespace: instance.GetNamespace()}, ssoSecret)
+				err = lutils.CustomizeEnvSSO(&deploy.Spec.Template, instance, r.GetClient(), r.IsOpenShift())


should reference statefulSet's template

will be fixed

leochr · 2020-05-21T16:48:35Z

pkg/controller/openliberty/openlibertyapplication_controller.go

 				if err != nil {
-					return errors.Wrapf(err, "Secret for Single sign-on (SSO) was not found. Create a secret named %q in namespace %q with the credentials for the login providers you selected in application image.", secretName, instance.GetNamespace())
+					reqLogger.Error(err, "Failed to reconcile Single sign-on configuration")
+					return err		


Use ManageError to handle error:
r.ManageError(err, common.StatusConditionTypeReconciled, instance)

this is inside the mutator function, the enclosing createOrUpdate call returns it, it's then handled with a call to r.ManageError outside of that around line 666, so ManageError is getting called.

leochr · 2020-06-05T15:12:13Z

doc/user-guide.adoc

 | `sso.github.hostname`   | Specifies the host name of your enterprise GitHub, such as _github.mycompany.com_. The default is _github.com_, which is the public Github.
 | `sso.oidc`   | The list of OpenID Connect (OIDC) providers to authenticate with. Required fields: _discoveryEndpoint_. Specify sensitive fields, such as _clientId_  and _clientSecret_, by using the `Secret`.
+| `sso.oidc[].autoRegisterSecret` | Specifies name of secret containing information to automatically register Liberty with the OIDC provider. See link:++#Using-automatic-registration-with-OIDC-providers++[Using automatic registration with OIDC providers].


Should it be "Liberty application"?

leochr · 2020-06-05T15:23:40Z

doc/user-guide.adoc

@@ -315,6 +318,66 @@ spec:
        name: self-signed
    termination: reencrypt
 ----
+==== Using automatic registration with OIDC providers
+
+The operator can request a client Id and client Secret from providers, rather than requiring them in advance. This can simplify deployment, as the provider's administrator can supply the information needed for registration once, instead of supplying clientIds and secrets repetitively.  An additional secret specified by `sso.oidc[].autoRegisterSecret` contains additional information necessary to perform the registration.  First the operator will make an https request to the `sso.oidc[].discoveryEndpoint` to obtain URLs for subsequent REST calls.  Next it will make additional REST calls to the provider and obtain a client Id and client Secret. Automatic registration takes precedence over the values in the sso secret. The name of this secret should end with `-olapp-sso` for it to be monitored for changes.  This is tested with Red Hat Single Sign-on (RHSSO). 


"The name of this secret should end with -olapp-sso for it to be monitored for changes."
With the current implementation you have in place, I don't think the auto-register secret is monitored.

Instead of users always having to explicitly specify the auto-register secret name, by default we could automatically find it (only while reconciling providers) using a naming convention: <CR_name>-olapp-sso-<provider_id>

I will look into auto-detecting off of the name, to make it consistent with the way we handle the other sso secret. I can't think of any disadvantage to that.

leochr · 2020-06-05T15:58:19Z

pkg/utils/utils.go

+			ssoEnv = append(ssoEnv, *createEnvVarSSO(id, "_CLIENTID", clientId))
+			ssoEnv = append(ssoEnv, *createEnvVarSSO(id, "_CLIENTSECRET", clientSecret))
+		} else {
+			if !haveSsoSecret {  // if we don't have an autoreg secret, we need an sso secret.


A similar error is needed for OAuth2 providers below and GitHub above (when Github.Hostname is specified).

leochr · 2020-06-05T16:25:38Z

pkg/utils/utils.go

+				}
+				clientId, clientSecret, err = RegisterWithOidcProvider(regData, regSecret)
+				if err != nil {
+					return errors.Wrapf(err, "Error occured during registration with OIDC provider")


It'll be good to add the provider ID here.

leochr · 2020-06-12T18:08:00Z

pkg/utils/utils.go

-				return errors.Wrapf(err, "Registration Data Secret for Single sign-on (SSO) of name %q was not found. Create a secret in namespace %q with the credentials for the login providers you selected in application image.", secretName, instance.GetNamespace())
-			}
+		// if no clientId specified for this provider, try auto-registration
+		clientId  := string(ssoSecret.Data[strings.ToLower(id) + "-clientId"])


There could be mismatch due to converting to lower case. oidcClient.ID should be used as is here.

leochr · 2020-06-12T18:47:27Z

pkg/utils/utils.go

+				return errors.Wrapf(err, "Error occured during registration with OIDC for provider " + providerId)
+			}
+
+			_, err = controllerutil.CreateOrUpdate(context.TODO(), client, ssoSecret, func() error {


Updating the secret for each provider will cause many reconciliations unnecessarily. It'll be good to collect the data to add to the secret and then update the secret just once at the end. That way it'll have to reconcile only once.

leochr · 2020-06-12T18:50:10Z

pkg/utils/utils.go

+				ssoSecret.Data[strings.ToLower(id) + autoregFragment + "RegisteredOidcClientId"] = []byte(clientId)
+				ssoSecret.Data[strings.ToLower(id) + autoregFragment + "RegisteredOidcSecret"] = []byte(clientSecret)
+				ssoSecret.Data[strings.ToLower(id) + "-clientId"] = []byte(clientId)
+				ssoSecret.Data[strings.ToLower(id) + "-clientSecret"] = []byte(clientSecret)


is the clientSecret already base64 encoded at this point? or will it be automatically converted when adding to secret?

kube appears to decode on read and encode on write, so we don't have to.

leochr · 2020-06-12T23:15:50Z

pkg/apis/openliberty/v1beta1/zz_generated.openapi.go

-		"github.com/OpenLiberty/open-liberty-operator/pkg/apis/openliberty/v1beta1.ServiceBindingConsumes":               schema_pkg_apis_openliberty_v1beta1_ServiceBindingConsumes(ref),
-		"github.com/OpenLiberty/open-liberty-operator/pkg/apis/openliberty/v1beta1.ServiceBindingProvides":               schema_pkg_apis_openliberty_v1beta1_ServiceBindingProvides(ref),
-		"github.com/OpenLiberty/open-liberty-operator/pkg/apis/openliberty/v1beta1.StatusCondition":                      schema_pkg_apis_openliberty_v1beta1_StatusCondition(ref),
+		"github.com/open-liberty-operator/pkg/apis/openliberty/v1beta1.GithubLogin":                          schema_pkg_apis_openliberty_v1beta1_GithubLogin(ref),


The path changed for some reason. Likely due to go path setting on your machine.

leochr

@brutif Looks good. We can merge this now. Thank you!

* update userguide for auto registration * add registration code * update types * clean up format * propagate error * interim updates * rework route logic * clean up * merge with integration * fix user-guide table * remove commented out code * minor doc clarifications * improve rhsso secret explanation * add liberty cr usage example * move registration results from status to the secret * update for code review comments * add updated crd * remove refs to autoreg secret that is no longer used * adjust registratiton json for IBM Cloud Verify * fix setting env var twice * updates for code review, change client name registered at provider * resolve merge conflict

brutif added 12 commits April 15, 2020 15:49

update userguide for auto registration

0bcdea1

add registration code

d4179db

update types

135a875

clean up format

39d6354

propagate error

be2079f

interim updates

b7f8f5c

rework route logic

042e64f

clean up

c15155e

resolve merge conflict

095fd66

merge with integration

b277706

fix user-guide table

f76b4b5

remove commented out code

14d2ada

brutif requested a review from leochr May 8, 2020 19:31

brutif mentioned this pull request May 12, 2020

IAM: Provide Support for Red Hat SSO in Open Liberty- Part 3 for Auto-registration kabanero-io/kabanero-security#71

Open

arthurdm reviewed May 27, 2020

View reviewed changes

brutif added 4 commits May 27, 2020 08:45

minor doc clarifications

855dc75

resolve merge conflict

af43ecd

improve rhsso secret explanation

5c39f20

add liberty cr usage example

ee59b55

move registration results from status to the secret

4b9bf76

leochr reviewed Jun 5, 2020

View reviewed changes

brutif added 5 commits June 9, 2020 09:53

update for code review comments

f1d13b3

add updated crd

4fd66f7

remove refs to autoreg secret that is no longer used

82da429

adjust registratiton json for IBM Cloud Verify

8f9d6bb

fix setting env var twice

e31a5e8

leochr reviewed Jun 12, 2020

View reviewed changes

updates for code review, change client name registered at provider

15e4685

brutif added 2 commits June 12, 2020 16:17

resolve merge conflict

325c964

resolve merge conflict, take2

26f2773

leochr reviewed Jun 12, 2020

View reviewed changes

leochr approved these changes Jun 12, 2020

View reviewed changes

brutif merged commit 5a2cb25 into OpenLiberty:master Jun 13, 2020

This was referenced Jun 15, 2020

Update SSO unit/e2e tests for recent changes #173

Closed

Fix SSO unit and e2e tests #174

Merged

Update SSO tests to cover auto-registration #178

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Oidc autoregistration #152

Oidc autoregistration #152

brutif commented May 8, 2020 •

edited

Loading

arthurdm May 27, 2020

brutif May 27, 2020

arthurdm May 28, 2020

brutif May 28, 2020 •

edited

Loading

arthurdm commented Jun 1, 2020

leochr left a comment

leochr May 21, 2020

brutif Jun 9, 2020

leochr May 21, 2020

brutif Jun 9, 2020

leochr Jun 5, 2020

brutif Jun 9, 2020

leochr Jun 5, 2020 •

edited

Loading

brutif Jun 5, 2020

leochr Jun 5, 2020

brutif Jun 9, 2020

leochr Jun 5, 2020

brutif Jun 9, 2020

leochr Jun 12, 2020

brutif Jun 12, 2020

leochr Jun 12, 2020

brutif Jun 12, 2020

leochr Jun 12, 2020

brutif Jun 12, 2020

leochr Jun 12, 2020

leochr left a comment •

edited

Loading

Oidc autoregistration #152

Oidc autoregistration #152

Conversation

brutif commented May 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brutif May 28, 2020 • edited Loading

Choose a reason for hiding this comment

arthurdm commented Jun 1, 2020

leochr left a 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

leochr Jun 5, 2020 • 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

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

leochr left a comment • edited Loading

Choose a reason for hiding this comment

brutif commented May 8, 2020 •

edited

Loading

brutif May 28, 2020 •

edited

Loading

leochr Jun 5, 2020 •

edited

Loading

leochr left a comment •

edited

Loading