How will user names and service account name collisions be handled?

[peternied]

Service accounts and user accounts use the same underlying storage system, so it possible for a user to intentionally create an account that is the name of a service account.

Exit Criteria

• What is the worst case scenario if this occurs?
• Do we need to prevent this?
  • NO - close this issue.
  • YES - Pull request to handle this scenario with some tests.

[stephen-crawford]

[Triage] Hi @peternied, thank you for filing this issue. This issue appears to have actionable tasks associated with its closure. Namely, investigating the status and required actions. Going to mark as triaged with completion on checklist completion.

[stephen-crawford]

There are three main scenarios which we can consider to decide how to handle this.

1. We have a service account with name SA_1 and we then have someone with access to the internal users api attempt to add a new user with the same name (SA_1).

We can see what happens in the scenario by looking at the UserService logic for createOrUpdateAccount.

Specifically, we see:

        final boolean userExisted = internalUsersConfiguration.exists(accountName);

        // sanity checks, hash is mandatory for newly created users
        if (!userExisted && securityJsonNode.get("hash").asString() == null) {
            throw new UserServiceException(NO_PASSWORD_OR_HASH_MESSAGE);
        }

        // for existing users, hash is optional
        if (userExisted && securityJsonNode.get("hash").asString() == null) {
            // sanity check, this should usually not happen
            final String hash = ((Hashed) internalUsersConfiguration.getCEntry(accountName)).getHash();
            if (hash == null || hash.length() == 0) {
                throw new UserServiceException(
                    "Existing user " + accountName + " has no password, and no new password or hash was specified."
                );
            }
            contentAsNode.put("hash", hash);
        }

        internalUsersConfiguration.remove(accountName);
        contentAsNode.remove("name");

        internalUsersConfiguration.putCObject(
            accountName,
            DefaultObjectMapper.readTree(contentAsNode, internalUsersConfiguration.getImplementingClass())
        );
        return internalUsersConfiguration;

Note that while we check whether the specified account already exists, we do not prevent modifying it. Part of the API functionality is the ability to use PUT to update an existing user and in this case, we would overwrite the service account with the user account.

Once this has happened, we will see that attempts to generate a new service account auth token will fail:

  try {
            DefaultObjectMapper mapper = new DefaultObjectMapper();
            JsonNode accountDetails = mapper.readTree(internalUsersConfiguration.getCEntry(accountName).toString());
            final ObjectNode contentAsNode = (ObjectNode) accountDetails;
            SecurityJsonNode securityJsonNode = new SecurityJsonNode(contentAsNode);

            Optional.ofNullable(securityJsonNode.get("service"))
                .map(SecurityJsonNode::asString)
                .filter("true"::equalsIgnoreCase)
                .orElseThrow(() -> new UserServiceException(AUTH_TOKEN_GENERATION_MESSAGE));

            Optional.ofNullable(securityJsonNode.get("enabled"))
                .map(SecurityJsonNode::asString)
                .filter("true"::equalsIgnoreCase)
                .orElseThrow(() -> new UserServiceException(AUTH_TOKEN_GENERATION_MESSAGE));

            // Generate a new password for the account and store the hash of it
            String plainTextPassword = generatePassword();
            contentAsNode.put("hash", hash(plainTextPassword.toCharArray()));
            contentAsNode.put("enabled", "true");
            contentAsNode.put("service", "true");

Further since resetting the user is all but guaranteed to change the password associated with the internal user account, any existing service account tokens based on the previously generated random password (how service account tokens are formed), will no longer work.

The extension could attempt to use the token, but when it tries to use BasicAuth to execute the action the password will no longer match the expected hash and it will fail. The only way the existing password would still work would be if the update to the user account kept the same hash intentionally. In this case, the token would still allow the extension to execute requests, but this seems addressable by documentation.

2. The second scenario to consider is if we already have an internal user account and were to create a service account for an extension that somehow matched the user account.

In this scenario, we have the inverse of the first option. Again, because we handle the creation of service accounts with the same methods that create or update an internal user account, we would just end up overwriting the internal user account.

This has the same considerations as above.

3. The final scenario to consider is if we have a manually defined internal user from the configuration files and they conflict with an extension's name.

In this scenario, the manually defined internal users would again be overridden.

We can see this behavior based on the order of initialization with the Security plugin and extensions.

In short, because the Security plugin will launch first, it will create the internal users from its configs. Then whenever an extension will come up, we will encounter scenario 2.

In my opinion, we should both very rarely encounter these scenarios--you would have to intentionally name an internal user something that would match an extensions unique id which should all start with "_"--and the potential outcomes of this conflict are restricted to broken behavior for either an extension attempting to use its service account or an internal user. In either case the extension or user would simply lose access as a result of the change in password.

You could correct this by resetting the service account variables to true and then generating a new Service Account token. You could correct the case for an internal user by resetting the password to what you would like and turning the service account variables to false.

[stephen-crawford]

I would like to close this issue based on the low stakes of a potential conflict. Waiting for agreement from others.

[peternied]

@scrawfor99 Thanks for spending the time to write this out. These seem pretty innocuous to me. I'm going to close out this issue. If there is concern, we can always reopen and address at that time.