Add support for Azure client certificate auth #2786

super-harsh · 2023-03-16T21:27:03Z

What this PR does / why we need it:

This PR adds support to handle client auth using Certificates

…zure-service-operator into feature/client-cert-auth

codecov-commenter · 2023-03-21T01:17:46Z

Codecov Report

Merging #2786 (71481df) into main (af6fb23) will decrease coverage by 0.01%.
The diff coverage is 9.67%.

@@            Coverage Diff             @@
##             main    #2786      +/-   ##
==========================================
- Coverage   52.49%   52.49%   -0.01%     
==========================================
  Files        1000     1001       +1     
  Lines      418800   418813      +13     
==========================================
+ Hits       219842   219843       +1     
- Misses     163731   163745      +14     
+ Partials    35227    35225       -2

Impacted Files	Coverage Δ
v2/identity/client_certificate_credential.go	`0.00% <0.00%> (ø)`
v2/internal/config/vars.go	`68.42% <ø> (ø)`
v2/internal/reconcilers/arm/arm_client_cache.go	`64.92% <13.04%> (-4.85%)`	⬇️

... and 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

matthchr

Basically LGTM, left a few small comments. Happy to approve once those are resolved.

docs/hugo/content/introduction/authentication.md

matthchr · 2023-03-21T16:46:18Z

docs/hugo/content/introduction/authentication.md

+export AZURE_CLIENT_ID="00000000-0000-0000-0000-00000000000"          # The client ID (sometimes called App Id) of the Service Principal.
+export AZURE_SUBSCRIPTION_ID="00000000-0000-0000-0000-00000000000"    # The Azure Subscription ID the identity is in.
+export AZURE_TENANT_ID="00000000-0000-0000-0000-00000000000"          # The Azure AAD Tenant the identity/subscription is associated with.
+export AZURE_CLIENT_CERTIFICATE="myCertificateValue"                  # The client certificate of the Service Principal.


minor: This should be the ASCII of the cert right? Can you make it look more like the cert? I think usually they will start with a header and then some values?

v2/charts/azure-service-operator/values.yaml

v2/cmd/controller/main.go

v2/test/single_operator_multitenant_test.go

Co-authored-by: Matthew Christopher <matthchr@users.noreply.github.com>

matthchr · 2023-03-22T15:44:55Z

docs/hugo/content/introduction/authentication.md

+export AZURE_CLIENT_ID="00000000-0000-0000-0000-00000000000"          # The client ID (sometimes called App Id) of the Service Principal.
+export AZURE_SUBSCRIPTION_ID="00000000-0000-0000-0000-00000000000"    # The Azure Subscription ID the identity is in.
+export AZURE_TENANT_ID="00000000-0000-0000-0000-00000000000"          # The Azure AAD Tenant the identity/subscription is associated with.
+export AZURE_CLIENT_CERTIFICATE="-----BEGIN PRIVATE KEY-----


minor: does this export command actually work? Not sure how multiline exports work.

yes, it does.. will have to do something like export AZURE_CLIENT_CERTIFICATE=cat certFile.pem`. Also, we have a livetest for this.

docs/hugo/content/introduction/authentication.md

theunrepentantgeek · 2023-03-22T23:40:22Z

docs/hugo/content/introduction/authentication.md

+
+### Prerequisites
+1. An existing Azure Service Principal.
+2. Certificate in ASCII format such as PEM, CER, or DER.


Any kind of certificate? Or are only certain algorithms/signing-authorities accepted? Probably too much detail to cover here, but a link would be useful.

docs/hugo/content/introduction/authentication.md

v2/charts/azure-service-operator/values.yaml

theunrepentantgeek · 2023-03-22T23:56:14Z

v2/cmd/controller/main.go

+	} else if cert := os.Getenv(config.ClientCertificateVar); cert != "" {
+		certPassword := os.Getenv(config.ClientCertificatePasswordVar)
+		credential, err = identity.NewClientCertificateCredential(cfg.TenantID, cfg.ClientID, []byte(cert), []byte(certPassword))
+		if err != nil {
+			return nil, errors.Wrapf(err, "unable to get client certificate credential")
+		}


This chain of if statements is unwieldy and challenging to read.

Suggest getting rid of the chaining by having each block unconditionally return. E.g.

if cfg.UseWorkloadIdentityAuth { credential, err = identity.NewWorkloadIdentityCredential(cfg.TenantID, cfg.ClientID) if err != nil { return nil, errors.Wrapf(err, "unable to get workload identity credential") } return credential, nil }

then the next block can stand alone

if cert := os.Getenv(config.ClientCertificateVar); cert != "" { ... elided }

and the default case doesn't need any nesting

credential, err = azidentity.NewDefaultAzureCredential(nil) if err != nil { return nil, errors.Wrapf(err, "unable to get default azure credential") } return credential, err

theunrepentantgeek · 2023-03-22T23:59:05Z

v2/internal/reconcilers/arm/arm_client_cache.go

 		credential, err = azidentity.NewClientSecretCredential(string(tenantID), string(clientID), string(clientSecret), nil)
 		if err != nil {
 			return nil, "", errors.Wrap(err, errors.Errorf("invalid Client Secret Credential for %q encountered", nsName.String()).Error())
 		}
+	} else if clientCert, hasClientCert := secret.Data[config.ClientCertificateVar]; hasClientCert {


Another case where chaining of the if statements makes it challenging for someone unfamiliar with the code. Maybe use a similar fix?

Co-authored-by: Bevan Arps <bevan.arps@microsoft.com>

* Add support for Azure client certificate auth Co-authored-by: Harshdeep Singh <harshdsingh@microsoft.com> Co-authored-by: Matthew Christopher <matthchr@users.noreply.github.com> Co-authored-by: Bevan Arps <bevan.arps@microsoft.com>

Harshdeep Singh added 6 commits March 17, 2023 10:09

Add support for Azure client certificate auth

9c0cce1

Update auth documentation steps for certificate auth

b069c9b

Merge branch 'main' into feature/client-cert-auth

aff1d3a

Merge branch 'feature/client-cert-auth' of https://github.com/Azure/a…

a3dae66

…zure-service-operator into feature/client-cert-auth

Update doc

863de32

Minor refactor

2d0250a

super-harsh marked this pull request as ready for review March 19, 2023 23:19

super-harsh requested review from davefellows, theunrepentantgeek, matthchr and babbageclunk as code owners March 19, 2023 23:19

Harshdeep Singh added 2 commits March 20, 2023 12:50

Add header

aab5c5f

Fiux linter issue

2c8918e

super-harsh self-assigned this Mar 20, 2023

Harshdeep Singh added 2 commits March 21, 2023 13:24

Remove extra line

ec27494

Add cert auth test

1eaf63a

Harshdeep Singh added 3 commits March 21, 2023 16:30

Improve test logging

e392f26

Add env vars for cert auth test

022a5e5

Tidy up

aeec1ab

matthchr reviewed Mar 21, 2023

View reviewed changes

super-harsh and others added 5 commits March 22, 2023 15:57

Update docs/hugo/content/introduction/authentication.md

b552aad

Co-authored-by: Matthew Christopher <matthchr@users.noreply.github.com>

Update docs/hugo/content/introduction/authentication.md

01c342e

Co-authored-by: Matthew Christopher <matthchr@users.noreply.github.com>

Update docs/hugo/content/introduction/authentication.md

f719cbf

Co-authored-by: Matthew Christopher <matthchr@users.noreply.github.com>

Minor changes requested by @matthchr

45025f0

Merge branch 'main' into feature/client-cert-auth

3b16923

matthchr approved these changes Mar 22, 2023

View reviewed changes

super-harsh and others added 3 commits March 23, 2023 09:58

Merge branch 'main' into feature/client-cert-auth

fb2762e

Update instructions

dd9770d

Merge branch 'main' into feature/client-cert-auth

dee04c2

theunrepentantgeek approved these changes Mar 23, 2023

View reviewed changes

super-harsh and others added 8 commits March 23, 2023 16:50

Update docs/hugo/content/introduction/authentication.md

3c20941

Co-authored-by: Bevan Arps <bevan.arps@microsoft.com>

Update v2/charts/azure-service-operator/values.yaml

1da14ec

Co-authored-by: Bevan Arps <bevan.arps@microsoft.com>

Update docs/hugo/content/introduction/authentication.md

5b37f25

Co-authored-by: Bevan Arps <bevan.arps@microsoft.com>

Update docs/hugo/content/introduction/authentication.md

9dd204a

Co-authored-by: Bevan Arps <bevan.arps@microsoft.com>

Update docs/hugo/content/introduction/authentication.md

a85b1f9

Co-authored-by: Bevan Arps <bevan.arps@microsoft.com>

Minor Refactor

5183c1b

Merge branch 'main' into feature/client-cert-auth

71481df

Update instructions

62b0374

super-harsh enabled auto-merge (squash) March 23, 2023 23:23

super-harsh merged commit 075bc3c into main Mar 23, 2023

super-harsh deleted the feature/client-cert-auth branch March 23, 2023 23:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Azure client certificate auth #2786

Add support for Azure client certificate auth #2786

super-harsh commented Mar 16, 2023 •

edited

codecov-commenter commented Mar 21, 2023 •

edited

matthchr left a comment

matthchr Mar 21, 2023

matthchr Mar 22, 2023

super-harsh Mar 22, 2023

theunrepentantgeek Mar 22, 2023

theunrepentantgeek Mar 22, 2023

theunrepentantgeek Mar 22, 2023

Add support for Azure client certificate auth #2786

Add support for Azure client certificate auth #2786

Conversation

super-harsh commented Mar 16, 2023 • edited

codecov-commenter commented Mar 21, 2023 • edited

Codecov Report

matthchr left a comment

Choose a reason for hiding this comment

matthchr Mar 21, 2023

Choose a reason for hiding this comment

matthchr Mar 22, 2023

Choose a reason for hiding this comment

super-harsh Mar 22, 2023

Choose a reason for hiding this comment

theunrepentantgeek Mar 22, 2023

Choose a reason for hiding this comment

theunrepentantgeek Mar 22, 2023

Choose a reason for hiding this comment

theunrepentantgeek Mar 22, 2023

Choose a reason for hiding this comment

super-harsh commented Mar 16, 2023 •

edited

codecov-commenter commented Mar 21, 2023 •

edited