Parse service account user from LDAP str #547

pjsier · 2023-11-07T23:19:24Z

Parse service account target user from LDAP string

The recent change in #525 to track target_user in state for minio_service_account is breaking for LDAP users. It looks like madmin is returning ParentUser as a full LDAP string such as "CN=minio-user,DC=example,DC=org" for minio-user.

This PR fixes that issue by attempting to parse each returned ParentUser string as an LDAP string, and if it fails return the string as-is. I saw some regex utilities for checking whether or not a string was LDAP, but it didn't look like they helped much with parsing, and since this ignores non-LDAP strings it felt simple enough to do this simply.

My big question is that I'm not sure how to test it other than the unit tests here. It looks like we don't have a ton of tools for testing LDAP users, so another set of eyes would be helpful

Reference

Partially resolves: minio_iam_service_account resource is updated on every run in 2.0.0 #544

acolombier · 2023-11-08T08:53:59Z

minio/resource_minio_service_account.go

+	for _, ldapSection := range strings.Split(parentUser, ",") {
+		splitSection := strings.Split(ldapSection, "=")
+		if len(splitSection) == 2 && strings.ToLower(strings.TrimSpace(splitSection[0])) == "cn" {
+			return strings.TrimSpace(splitSection[1])
+		}
+	}


AFAIK, a CN can only be a leaf in a LDAP/AD directory, so there is no need to iterate over each node. Furthermore, = may be a legal character if it has been escaped (see RFC4514, page 13). How about using a regex instead?

Finally, potentially out of scope for this issue, but the string may contain sequence character U+... but I'm not sure it would be interpreted as valid unicode char code.

@acolombier thanks for the clarification and link to the RFC! I'm not familiar with the specification so this is helpful. Just skimming the spec for now, but is = allowed to have spaces around it after a section? i.e. is CN = test valid?

acolombier · 2023-11-08T08:59:06Z

minio/resource_minio_service_account_test.go

@@ -147,6 +148,12 @@ func TestServiceAccount_Policy(t *testing.T) {
 	})
 }

+func TestParseUserFromParentUser(t *testing.T) {
+	assert.Equal(t, "minio-user", parseUserFromParentUser("minio-user"))
+	assert.Equal(t, "minio-user", parseUserFromParentUser("CN = minio-user, DC=example,DC=org"))


If my interpretation of the RFC4514 (page 11) is correct, this should result in minio-user

felladrin · 2023-12-07T11:03:29Z

I don't know how to test, but as no one is against this change and no one else has proposed a different solution, we should try it.

ℹ️ For releasing a new version, all we need to do is push a new tag (in this case it would be v1.20.2), and the CI will create a release here on GitHub, which in turn will trigger an update in Terraform Registry via webhook :) Could you do it after merging this, @pjsier?

pjsier · 2024-01-06T20:12:58Z

@felladrin Sorry for the delay! Can we cut a beta release on this so that it's easier to test but not a full release?

felladrin · 2024-03-11T18:37:43Z

Sorry for being also absent. It's getting harder and harder to find a gap. We need new collaborators :D

Currently, we don't have a way to do beta releases, but I love the idea. Will check if it's possible.

pjsier · 2024-03-11T18:45:17Z

It looks like you can use sources from GitHub commits? That might be the safest way to test and I’ll update the issue

and no problem! I’ve also been swamped

felladrin · 2024-03-11T19:06:21Z

Thanks!
After testing it, will you create the tag v2.2.0 on ac81376? :)

pjsier · 2024-03-12T00:01:37Z

@felladrin that was the original issue is it wasn’t clear how to test this one because we couldn’t reproduce it, so we would need someone experiencing it

felladrin · 2024-03-12T13:40:34Z

I think we might have a chicken-egg problem here. We're hesitant to release it because we're waiting for someone to test it, but no one will test it until we release it, as they probably won't build it just to test it.

I've just merged another PR that I think is worth releasing along with this.
Let me tag v2.2.0 on the last commit from main and we get the feedback from users this time, ok?

About releasing a beta version, I've checked that Terraform Registry doesn't provide a way to create pre-releases, and what others are doing is creating a second account just to release the beta releases. (e.g. Google / Google-Beta). But it also requires having two different repositories on GitHub.

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [minio](https://registry.terraform.io/providers/aminueza/minio) ([source](https://togithub.com/aminueza/terraform-provider-minio)) | required_provider | minor | `2.0.1` -> `2.2.0` | --- ### Release Notes <details> <summary>aminueza/terraform-provider-minio (minio)</summary> ### [`v2.2.0`](https://togithub.com/aminueza/terraform-provider-minio/releases/tag/v2.2.0) [Compare Source](https://togithub.com/aminueza/terraform-provider-minio/compare/v2.1.0...v2.2.0) #### What's Changed - Parse service account user from LDAP str by [@pjsier](https://togithub.com/pjsier) in [aminueza/terraform-provider-minio#547 - Bump github.com/cloudflare/circl from 1.3.3 to 1.3.7 by [@dependabot](https://togithub.com/dependabot) in [aminueza/terraform-provider-minio#560 **Full Changelog**: aminueza/terraform-provider-minio@v2.1.0...v2.2.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate).  Co-authored-by: kireque-bot[bot] <143391978+kireque-bot[bot]@users.noreply.github.com>

Parse service account user from LDAP str

025cf55

pjsier mentioned this pull request Nov 7, 2023

minio_iam_service_account resource is updated on every run in 2.0.0 #544

Open

acolombier reviewed Nov 8, 2023

View reviewed changes

felladrin approved these changes Dec 7, 2023

View reviewed changes

felladrin merged commit ac81376 into aminueza:main Mar 11, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse service account user from LDAP str #547

Parse service account user from LDAP str #547

pjsier commented Nov 7, 2023

acolombier Nov 8, 2023

pjsier Nov 15, 2023

acolombier Nov 8, 2023

felladrin commented Dec 7, 2023

pjsier commented Jan 6, 2024

felladrin commented Mar 11, 2024

pjsier commented Mar 11, 2024 •

edited

felladrin commented Mar 11, 2024

pjsier commented Mar 12, 2024

felladrin commented Mar 12, 2024 •

edited

Parse service account user from LDAP str #547

Parse service account user from LDAP str #547

Conversation

pjsier commented Nov 7, 2023

Parse service account target user from LDAP string

Reference

acolombier Nov 8, 2023

Choose a reason for hiding this comment

pjsier Nov 15, 2023

Choose a reason for hiding this comment

acolombier Nov 8, 2023

Choose a reason for hiding this comment

felladrin commented Dec 7, 2023

pjsier commented Jan 6, 2024

felladrin commented Mar 11, 2024

pjsier commented Mar 11, 2024 • edited

felladrin commented Mar 11, 2024

pjsier commented Mar 12, 2024

felladrin commented Mar 12, 2024 • edited

pjsier commented Mar 11, 2024 •

edited

felladrin commented Mar 12, 2024 •

edited