[Merged by Bors] - Kerberos AD backend #254

nightkr · 2023-03-17T13:30:56Z

Description

Fixes #253

Definition of Done Checklist

Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
Please make sure all these things are done and tick the boxes

# Author
- [ ] Changes are OpenShift compatible
- [ ] CRD changes approved
- [ ] Helm chart can be installed and deployed operator works
- [ ] Integration tests passed (for non trivial changes)

# Reviewer
- [ ] Code contains useful comments
- [ ] (Integration-)Test cases added
- [ ] Documentation added or updated
- [ ] Changelog updated
- [ ] Cargo.toml only contains references to git tags (not specific commits or branches)

# Acceptance
- [ ] Feature Tracker has been updated
- [ ] Proper release label has been added

Once the review is done, comment bors r+ (or bors merge) to merge. Further information

… nodes

nightkr · 2023-03-27T13:36:17Z

Note before merging this: currently this disables TLS validation, need to let users supply a trust root instead.

sbernauer · 2023-03-28T06:08:28Z

Successfully tested this branch against a Windows Server 2022 AD and stackabletech/hdfs-operator#334

sbernauer · 2023-03-28T06:53:37Z

I run into the following issue:

2023-03-28T06:46:48.462981Z  INFO stackable_krb5_provision_keytab: Creating principal principal="jn/hdfs-journalnode-default-0.hdfs-journalnode-default.kuttl-test-wired-rat.svc.cluster.local"                                                                                                                                           
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: LdapResult { result: LdapResult { rc: 68, matched: "", text: "00002071: UpdErr: DSID-03050438
, problem 6005 (ENTRY_EXISTS), data 0\n\0", refs: [], ctrls: [] } }', rust/krb5-provision-keytab/src/main.rs:259:22                                                  
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

k -n kuttl-test-wired-rat get secret secret-operator-ad-passwords -o yaml
data:
  dn--hdfs-datanode-default-0.hdfs-datanode-default.kuttl-test-wired-rat.svc.cluster.local--EXAMPLE.COM: RU5FUDVtJzNsXzZRZFhNYS8nIVwlRkRicS1FfHZfOGd8O0xueUlZYA==
  dn--hdfs-datanode-default.kuttl-test-wired-rat.svc.cluster.local--EXAMPLE.COM: cV1fYHohTjRbbXQ6e2A5fklDUkU2fT03JT0lTTd6cFg+Mjkldm40RQ==
  jn--hdfs-journalnode-default.kuttl-test-wired-rat.svc.cluster.local--EXAMPLE.COM: KD0ueGZbNyQ0TCl6eFdLcD1GTDFiUFEnd08ySlF5bkx5UitHbTZfNA==
  nn--hdfs-namenode-default-0.hdfs-namenode-default.kuttl-test-wired-rat.svc.cluster.local--EXAMPLE.COM: SSx2JU89UVtmLDJqO0phTl1fdjsjVmJQMkNJWil6VnQpYUpFQ05tVQ==
  nn--hdfs-namenode-default.kuttl-test-wired-rat.svc.cluster.local--EXAMPLE.COM: Nl99bnV1WEY5ZS5RemIsQWNscDddWTpFKE9MeXNXU1xsNUQmJnp7Rg==

Theese are all the avaialble JN principals

You can see my AD contains users from a previous testrun but not the user jn/hdfs-journalnode-default-0.hdfs-journalnode-default.kuttl-test-wired-rat.svc.cluster.local (only jn/hdfs-journalnode-default-0.hdfs-journalnode-default.kuttl-test-clever-hog.svc.cluster.local is already in the AD)

sbernauer · 2023-03-28T07:14:04Z

I think I'm stupid and it actually tries to create dn/kind-control-plane, which names clash (as of the node scope)
Because the Secret containing the cashed passwords is located in the kuttl namespace it get's deleted while the AD user still existing.
What do you suggest?

Remove the node scope from hdfs-op?
Somehow fix this (haven't thought too much yet)
a. Force creation of new user
b. Override pw of the user and store it in the Secret
b. Somehow read the password and put it in the Secret (I guess this is not possible)
Something else

Do we want to improve the error message, so that we know which user can't be created?

2023-03-28T07:08:51.977954Z  INFO stackable_krb5_provision_keytab: Creating principal principal="dn/kind-control-plane"                                              
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: LdapResult { result: LdapResult { rc: 68, matched: "", text: "00002071: UpdErr: DSID-03050438
, problem 6005 (ENTRY_EXISTS), data 0\n\0", refs: [], ctrls: [] } }', rust/krb5-provision-keytab/src/main.rs:259:22                                                  
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

nightkr · 2023-03-28T07:24:33Z

There might have been a race condition, it can also happen if we fail to write the password back into K8s after creating the AD user...

sbernauer · 2023-03-28T07:26:31Z

That sucks!
Deleting {HTTP,nn}/kind-control-plane users solved the problem for me

sbernauer

Did walk through the code.
Please don't forget to add a breaking changelog entry

docs/modules/secret-operator/pages/secretclass.adoc

rust/krb5-provision-keytab/Cargo.toml

rust/krb5-provision-keytab/src/main.rs

tests/templates/kuttl/kerberos/secretclass.yaml

tests/test-definition.yaml

nightkr · 2023-03-28T12:40:49Z

I think I'd like to get the CRD change into 23.4, which would make the rest of this PR non-breaking when it does land.

sbernauer · 2023-03-28T12:59:20Z

I think I'd like to get the CRD change into 23.4, which would make the rest of this PR non-breaking when it does land.

+1, was thinking the same

# Description This should help prepare for making AD integration (#254) a non-breaking change. Co-authored-by: Natalie <[email protected]>

sbernauer · 2023-03-31T11:29:57Z

Regarding b25995d: Do we also want so support webpki as ca-cert validation mechanism (as we e.g. do for ldap server conections). Or would say 99% of the users run their AD behind their own pki?

nightkr · 2023-03-31T11:32:43Z

I would assume that private PKI is by far the norm. We can always go back and enable WebPKI support later, should the need arise.

sbernauer

Thanks for working on this! Only two minor comments left

sbernauer · 2023-05-19T10:47:57Z

docs/modules/secret-operator/pages/secretclass.adoc

+          # ldapServer must match the AD Domain Controller's FQDN or GSSAPI authn will fail
+          # You may need to set AD as your fallback DNS resolver in your Kube DNS Corefile
+          ldapServer: addc.example.com
+          passwordCacheSecret:


Can we please also document

ldapTlsCaSecret: # namespace: default name: secret-operator-ad-ca

Also that we will always use ldaps and the Secret needs the key ca.crt

rust/krb5/src/lib.rs

sbernauer · 2023-05-19T11:00:14Z

tests/test-definition.yaml

@@ -3,3 +3,6 @@ dimensions: []
 tests:
  - name: kerberos
    dimensions: []
+  # Requires manual connection to an AD cluster
+  - name: kerberos-ad


Do you want to disable this test by default before merging?

Yes, I think that makes sense to do.

sbernauer

LGTM % the documentation comment

sbernauer

Thx!

nightkr · 2023-05-19T11:49:27Z

bors r+

# Description Fixes #253 Co-authored-by: Sebastian Bernauer <[email protected]>

bors · 2023-05-19T12:01:46Z

Pull request successfully merged into main.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

All tests passed

nightkr added 16 commits March 17, 2023 14:29

Initial prototype of Kerberos AD backend

4d57778

Drop gdb leftovers

f51ba19

Remove keylogging leftovers

af2efb4

Use GSSAPI bind to authenticate to AD LDAP

b10b6d1

Cache AD credentials

dbabf97

Make AD password cache location configurable

6c6b09a

The service principals should conflict anyway, no need to worry about…

b129829

… nodes

Document concurrency principle for cache...

5bcc0d8

Document that the ADDC FQDN must match

b14977c

Add AD to documentation

10116ca

Generate random AD passwords

889e0a1

Unparse principal names properly rather than using string concatenation

0362519

Allow users to set custom user/schema root DNs

ba33ff3

Maybe yamllint prefers this?

c4f44c0

Update CRD

aacb1f4

Provision SPN and set supported encryption types

17efc29

sbernauer reviewed Mar 28, 2023

View reviewed changes

nightkr mentioned this pull request Mar 29, 2023

[Merged by Bors] - Split out MIT into separate SecretClass CRD section #257

Closed

bors bot pushed a commit that referenced this pull request Mar 30, 2023

Split out MIT into separate SecretClass CRD section (#257)

bdc4462

# Description This should help prepare for making AD integration (#254) a non-breaking change. Co-authored-by: Natalie <[email protected]>

nightkr added 3 commits March 30, 2023 17:02

Split Kerberos provisioning backends into separate modules

e098340

Don't unwrap AD init result

e303631

Add TLS verification for AD LDAP

b25995d

nightkr added 11 commits March 31, 2023 14:18

Factor out AD credential cache

642402d

Oops, forgot to actually include the credential cache files

215b1f1

Factor out AD user creation a bit

e270fa2

Merge branch 'main' into feature/kerberos-ad

383a516

Mention in docs that AD is supported

9b73456

Reenable MIT krb test, use test ns in secretclass names

8d28a9d

Another smidge of error handling

792c0b7

Document new krb5 additions

bcc6208

Try to clarify test instructions a bit

e720a84

Merge branch 'main' into feature/kerberos-ad

64ea3b1

Use __ as the AD cache separator

9295004

nightkr marked this pull request as ready for review April 17, 2023 13:23

nightkr requested review from sbernauer and a team April 17, 2023 13:23

Merge remote-tracking branch 'origin/main' into feature/kerberos-ad

0fb41a6

sbernauer requested changes May 19, 2023

View reviewed changes

sbernauer reviewed May 19, 2023

View reviewed changes

sbernauer approved these changes May 19, 2023

View reviewed changes

nightkr added 2 commits May 19, 2023 13:40

Document that AD requires a TLS CA

77c0c05

Disable kerberos-ad test by default

723898c

nightkr requested a review from sbernauer May 19, 2023 11:42

sbernauer approved these changes May 19, 2023

View reviewed changes

bors bot pushed a commit that referenced this pull request May 19, 2023

Kerberos AD backend (#254)

04f680a

# Description Fixes #253 Co-authored-by: Sebastian Bernauer <[email protected]>

bors bot changed the title ~~Kerberos AD backend~~ [Merged by Bors] - Kerberos AD backend May 19, 2023

bors bot closed this May 19, 2023

bors bot deleted the feature/kerberos-ad branch May 19, 2023 12:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Kerberos AD backend #254

[Merged by Bors] - Kerberos AD backend #254

nightkr commented Mar 17, 2023 •

edited

Loading

nightkr commented Mar 27, 2023

sbernauer commented Mar 28, 2023

sbernauer commented Mar 28, 2023

sbernauer commented Mar 28, 2023 •

edited

Loading

nightkr commented Mar 28, 2023

sbernauer commented Mar 28, 2023

sbernauer left a comment

nightkr commented Mar 28, 2023

sbernauer commented Mar 28, 2023

sbernauer commented Mar 31, 2023

nightkr commented Mar 31, 2023 •

edited

Loading

sbernauer left a comment

sbernauer May 19, 2023

nightkr May 19, 2023

sbernauer May 19, 2023

nightkr May 19, 2023

nightkr May 19, 2023

sbernauer left a comment

sbernauer left a comment

nightkr commented May 19, 2023

bors bot commented May 19, 2023

[Merged by Bors] - Kerberos AD backend #254

[Merged by Bors] - Kerberos AD backend #254

Conversation

nightkr commented Mar 17, 2023 • edited Loading

Description

Definition of Done Checklist

nightkr commented Mar 27, 2023

sbernauer commented Mar 28, 2023

sbernauer commented Mar 28, 2023

sbernauer commented Mar 28, 2023 • edited Loading

nightkr commented Mar 28, 2023

sbernauer commented Mar 28, 2023

sbernauer left a comment

Choose a reason for hiding this comment

nightkr commented Mar 28, 2023

sbernauer commented Mar 28, 2023

sbernauer commented Mar 31, 2023

nightkr commented Mar 31, 2023 • edited Loading

sbernauer left a comment

Choose a reason for hiding this comment

sbernauer May 19, 2023

Choose a reason for hiding this comment

nightkr May 19, 2023

Choose a reason for hiding this comment

sbernauer May 19, 2023

Choose a reason for hiding this comment

nightkr May 19, 2023

Choose a reason for hiding this comment

nightkr May 19, 2023

Choose a reason for hiding this comment

sbernauer left a comment

Choose a reason for hiding this comment

sbernauer left a comment

Choose a reason for hiding this comment

nightkr commented May 19, 2023

bors bot commented May 19, 2023

nightkr commented Mar 17, 2023 •

edited

Loading

sbernauer commented Mar 28, 2023 •

edited

Loading

nightkr commented Mar 31, 2023 •

edited

Loading