LDAP Auth user/group mapping are cleared when loading from a file with a name containing an equal ('=')

[arusso]

Describe the bug

When creating / updating an LDAP Auth group (or user) mapping from the command-line using the load from file (@) where the filename contains an equal (=), the group is either created or updated but the policies in the mapping are emptied.

To Reproduce

# setup config, works here!
❯ vault auth enable -path ldap-test ldap
Success! Enabled ldap auth method at: ldap-test/

❯ cat ldap-config.json
{
  "anonymous_group_search": false,
  "binddn": "",
  "case_sensitive_names": true,
  "certificate": "-----BEGIN...",
  "deny_null_bind": true,
  "discoverdn": true,
  "groupattr": "memberOf",
  "groupdn": "ou=people,o=example.com",
  "groupfilter": "(uid={{.Username}})",
  "insecure_tls": false,
  "starttls": false,
  "tls_max_version": "tls12",
  "tls_min_version": "tls10",
  "token_bound_cidrs": [],
  "token_explicit_max_ttl": 0,
  "token_max_ttl": 0,
  "token_no_default_policy": false,
  "token_num_uses": 0,
  "token_period": 0,
  "token_policies": [],
  "token_ttl": 0,
  "token_type": "default",
  "upndomain": "",
  "url": "ldaps://ldap-infra.example.com:636",
  "use_pre111_group_cn_behavior": true,
  "use_token_groups": false,
  "userattr": "uid",
  "userdn": "ou=people,o=example.com"
}

❯ vault write auth/ldap-test/config @ldap-config.json
Success! Data written to: auth/ldap-test/config

❯ vault read -format=json -field=data auth/ldap-test/config
{
  "anonymous_group_search": false,
  "binddn": "",
  "case_sensitive_names": true,
  "certificate": "-----BEGIN...",
  "deny_null_bind": true,
  "discoverdn": true,
  "groupattr": "memberOf",
  "groupdn": "ou=people,o=example.com",
  "groupfilter": "(uid={{.Username}})",
  "insecure_tls": false,
  "starttls": false,
  "tls_max_version": "tls12",
  "tls_min_version": "tls10",
  "token_bound_cidrs": [],
  "token_explicit_max_ttl": 0,
  "token_max_ttl": 0,
  "token_no_default_policy": false,
  "token_num_uses": 0,
  "token_period": 0,
  "token_policies": [],
  "token_ttl": 0,
  "token_type": "default",
  "upndomain": "",
  "url": "ldaps://ldap-infra.example.com:636",
  "use_pre111_group_cn_behavior": true,
  "use_token_groups": false,
  "userattr": "uid",
  "userdn": "ou=people,o=example.com"
}

# group mapping, does not work here
❯ cat cn=terraform-users,ou=groups,o=example.com.json
{
  "policies": [
    "role.terraform"
  ]
}

❯ vault write auth/ldap-test/groups/cn=terraform-users,ou=groups,o=example.com @cn=terraform-users,ou=groups,o=example.com.json
Success! Data written to: auth/ldap-test/groups/cn=terraform-users,ou=groups,o=example.com

# no error, policies wiped!!!
❯ vault read auth/ldap-test/groups/cn=terraform-users,ou=groups,o=example.com
Key         Value
---         -----
policies    []

❯ cat cn=terraform-users,ou=groups,o=example.com.json | vault write auth/ldap-test/groups/cn=terraform-users,ou=groups,o=example.com -
Success! Data written to: auth/ldap-test/groups/cn=terraform-users,ou=groups,o=example.com

❯ vault read auth/ldap-test/groups/cn=terraform-users,ou=groups,o=example.com
Key         Value
---         -----
policies    [role.terraform]

❯ vault read -format=json -field=data auth/ldap-test/groups/cn=terraform-users,ou=groups,o=example.com
{
  "policies": [
    "role.terraform"
  ]
}

Expected behavior
I would expect that the policies be updated regardless of the method we load our json config. If there's an error occurring somewhere, the policies shouldn't be wiped and an error should be returned.

Environment:

❯ vault status
Key             Value
---             -----
Seal Type       shamir
Initialized     true
Sealed          false
Total Shares    1
Threshold       1
Version         1.6.3
Storage Type    inmem
Cluster Name    vault-cluster-0ecb6e22
Cluster ID      44cc499b-0e56-6c20-af5f-97a8cd8af1a0
HA Enabled      false

❯ vault version
Vault v1.6.3 (b540be4b7ec48d0dd7512c8d8df9399d6bf84d76)

❯ uname -a
Linux helicarrier 3.10.0-1160.25.1.el7.x86_64 #1 SMP Tue Apr 13 18:55:45 EDT 2021 x86_64 x86_64 x86_64 GNU/Linux

Vault server configuration file(s):

Example above is using the vault server -dev -log-level=err, so whatever the defaults are.

Additional context

This behavior seems to affect user mappings as well.

[arusso]

This seems to actually be an issue with go-secure-stdlib

Because the split on = is occurring before the check to see if @ is the first character, and the file path is a dn with included = the library seems to be interpreting the input as {"@cn": "terraform-admins,o=example.com.json"} instead of reading from the file.

I'll file an issue there separately.

[pmmukh]

Hi @arusso ! Thank you for filing this bug! I just reproduced this locally, and can confirm that this is a valid issue. Will get started on a fix soon, and will update here once done.

[arusso]

I believe this affects more than the ldap auth backend (though I haven’t tested) so it may be worth exploring that and see if there’s a way to generalize the fix.

[pmmukh]

Agreed! The issue is related to how we generally parse these inputs on the command line, as I think you've probably seen already https://github.com/hashicorp/vault/blob/main/command/base_helpers.go#L132. I'm not fully clear right now on what our exact rules are around these inputs, i'm following up internally to get clarity around that, but yes, I do believe any solution we come up with here will apply broadly, as it'll lie in the go-secure-stdlib library.

[pmmukh]

Hi! So i followed up internally, and the consensus is that we want to leave the code right now as is, cause changing it to move the file parsing bit above the split on = bit, would mean that we wouldn't support key value pairs like @key=value, which we currently do, and given this has been the behavior of the argument parser for a while now, that might be a breaking change. We haven't explicitly called out in the documentation that = in the filename is unsupported, so I'll be making a docs change to that effect to mark it as such, and will keep the issue open till then. Sorry for the confusion!!

[arusso]

Given that the resulting map doesn’t have any of the keys to properly configure the ldap auth backend, shouldn’t at least an error be returned?

…

On Fri, Aug 20, 2021 at 14:01 Pratyoy Mukhopadhyay ***@***.***> wrote: Hi! So i followed up internally, and the consensus is that we want to leave the code right now as is, cause changing it to move the file parsing bit above the split on = bit, would mean that we wouldn't support key value pairs like @key <https://github.com/key>=value, which we currently do, and given this has been the behavior of the argument parser for a while now, that might be a breaking change. We haven't explicitly called out in the documentation that = in the filename is unsupported, so I'll be making a docs change to that effect to mark it as such, and will keep the issue open till then. Sorry for the confusion!! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12290 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEJGTIJMFMBVM54B6OGOZDT527EFANCNFSM5B2XNI7A> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email> .

[pmmukh]

Ah yes that is correct, an error should be returned here! I'm going to mark this as a bug for now, and keep this open till we get in a fix for the kv-builder to return an error when both @ and = are present in an argument

[pmmukh]

@arusso so after chatting with the team about maybe having the flow you ran into return an error, we've concluded that for now we'd like the keep the existing behavior as is. This is because in these cases, where an endpoint is given an invalid key ( in this case, @cn ), vault does not typically return an error. So changing this behavior to validate inputs would be a fairly large and breaking change ( and something we'd like to do across the board instead of in an endpoint-specific way ), and while there is some interest in dealing with this in the future, currently we don't have any plans for this. I've updated the documentation to be clearer on this edge case ( thanks again for pointing it out ), and will go ahead and close out this issue now, as I don't think any further action is required on this right now.