Parse service account user from LDAP str #547
Conversation
| 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]) | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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?
| @@ -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")) | |||
There was a problem hiding this comment.
If my interpretation of the RFC4514 (page 11) is correct, this should result in minio-user
|
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 |
|
@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? |
|
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. |
|
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 |
|
Thanks! |
|
@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 |
|
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. 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 [https://github.com/aminueza/terraform-provider-minio/pull/547](https://togithub.com/aminueza/terraform-provider-minio/pull/547) - Bump github.com/cloudflare/circl from 1.3.3 to 1.3.7 by [@​dependabot](https://togithub.com/dependabot) in [https://github.com/aminueza/terraform-provider-minio/pull/560](https://togithub.com/aminueza/terraform-provider-minio/pull/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. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yMzkuMCIsInVwZGF0ZWRJblZlciI6IjM3LjI0Ni4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9--> Co-authored-by: kireque-bot[bot] <143391978+kireque-bot[bot]@users.noreply.github.com>
Parse service account target user from LDAP string
The recent change in #525 to track
target_userin state forminio_service_accountis breaking for LDAP users. It looks likemadminis returningParentUseras a full LDAP string such as"CN=minio-user,DC=example,DC=org"forminio-user.This PR fixes that issue by attempting to parse each returned
ParentUserstring 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