Reworked API key management to use internal IDs instead of API keys #99

billkalter · 2017-03-14T19:01:00Z

Github Issue

50 (partial)

What Are We Doing Here?

The current API administration task uses the API key itself as input. However, API keys are supposed to be secret, and in our experience user's can't be trusted not to communicate their API key over a public channel when they need new permissions, such as by posting the key in a chat client or an email.

Additionally, as Emo moves toward distributed API key and permission management there will be a separation between individuals who grant specific roles. For example, you may go to team A to get permission to read from tables "team_a:*" while you may go to team B to get permission to read from tables "team_b:*". In both cases you may not trust giving your secret credential to anyone on either of those teams, but with the current API there is no alternative but to do so in order to for them to grant your API key a role with the new permissions.

This PR changes the API administration task to operate on the immutable internal IDs instead of the keys themselves, now generically referred to as "authentication IDs" or, for API keys in particular, "keys". So as not to break existing code or data the IDs are still referred to as internalId, even though it is used both internally and externally as a non-secret public identifier.

This PR consists of two commits. The first is the actual work for this PR. The second is simply a rename of RoleUpdateRequest to RoleModification. This was done for two reasons. The first is simply to mirror the class name AuthIdentityModification. The second is that RoleUpdateRequest was being used to both create and update roles, so it was a bit of a misnomer. The new name is more generic and less confusingly applies to both operations, though if there's a better name for it please make a comment.

A few notes:

Even though the effect of the change is relatively minor as a side effect it required rewriting the API for AuthIdentityManager to work on internal IDs instead of authentication IDs. As such this PR encompasses more changes than I would have thought necessary at the outset, but ultimately I think it flows well and closes a possible vulnerability in that it no longer stores the unencrypted API key as an attribute of the ApiKey class.
Another side effect of this change is that the logic for migrating API keys needed to be pushed down into AuthIdentityManager. This is actually a change for the better since it allows for an atomic migrate operation, and arguably it should have been implemented this way in the first place.
As with role management, a new DataCenterSynchronizedAuthIdentityManager was introduced to ensure each mutative API key operation is serializable within the local data center. For now this can be managed by only running API key tasks within one data center; in the future more changes could be added to enforce this.

How to Test and Verify

Check out this PR
Create roles and API keys using the respective tasks
Verify that all operations behave as expected.

Risk

Risk is relatively low. The format of the data actually stored in the authentication tables __auth:keys and __auth:internal_ids is unchanged, so there shouldn't be a migration issue. The biggest risk is regression, that a change in the API key manager breaks permission checks.

Level

Medium

Required Testing

Regression

Code Review Checklist

Tests are included. If not, make sure you leave us a line or two for the reason.
Pulled down the PR and performed verification of at least being able to
build and run.
Well documented, including updates to any necessary markdown files. When
we inevitably come back to this code it will only take hours to figure out, not
days.
Consistent/Clear/Thoughtful? We are better with this code. We also aren't
a victim of rampaging consistency, and should be using this course of action.
We don't have coding standards out yet for this project, so please make sure to address any feedback regarding STYLE so the codebase remains consistent.
PR has a valid summary, and a good description.

bv-jenkins · 2017-03-14T21:21:39Z

This PR was tested by custom branch build on jenkins

…I key management

bv-jenkins · 2017-03-17T15:27:32Z

This PR was tested by custom branch build on jenkins

sujithvaddi

@billkalter Change looks good to me 👍 minor comments posted, otherwise it's all good.

Verified the following:
change to APIKey class.
change to the AuthIdentityManager interface.
Implementations of AuthIdentityManager. Dao impl - Identity table -> <hash_auth_id, Identity Object> & index table -> <Internal_id, hash_auth_id>

sujithvaddi · 2017-03-27T03:59:29Z

auth/auth-core/src/main/java/com/bazaarvoice/emodb/auth/apikey/ApiKeyRealm.java

@@ -418,13 +418,13 @@ private void cacheAuthorizationInfoByInternalId(String internalId, Authorization
     */
    private AuthorizationInfo getUncachedAuthorizationInfoByInternalId(String internalId) {
        // Retrieve the roles by internal ID


minor: missed the comment update.

sujithvaddi · 2017-03-27T04:06:57Z

auth/auth-core/src/main/java/com/bazaarvoice/emodb/auth/apikey/ApiKeyAuthenticationInfo.java

@@ -17,14 +17,14 @@
    private final PrincipalCollection _principals;
    private final String _credentials;

-    public ApiKeyAuthenticationInfo(ApiKey apiKey, String realm) {
+    public ApiKeyAuthenticationInfo(String authenticationId, ApiKey apiKey, String realm) {


minor: null check for authenticationId.

sujithvaddi · 2017-03-27T04:11:23Z

auth/auth-core/src/main/java/com/bazaarvoice/emodb/auth/identity/AuthIdentity.java

+     *
+     * In the original design this ID was only used internally, but over time it has become an accepted public-facing
+     * identifier for identities.  This is why for historical reasons this field is called "internalId" even though
+     * it is used as both the unique internal and external identifier for this identity.
     */
    private final String _internalId;



I've seen your note about the reason for keeping the InternalID naming as is, but if it's only about changing the name at various places, I'm more biased in changing it, as it's now used as an external identifier as well. But it's your call though.

The reason I didn't change it is because the change cascades down. Notably, to be consistent I'd have to change Subject.java and InternalAuthorizer.java, as well as any classes which use them. I agree with your statement and don't mind doing it, but it makes this PR all the larger.

sujithvaddi · 2017-03-27T05:49:38Z

auth/auth-core/src/main/java/com/bazaarvoice/emodb/auth/identity/AuthIdentityModification.java

+     *          .withOwner("[email protected]")
+     *          .addRoles("role1")
+     *          .buildFrom("123");
+     *


minor: I think it's buildNew("123") here.

billkalter · 2017-03-28T18:29:41Z

@sujithvaddi I addressed your feedback. There's two commits; the first is small and contains the smaller updates from your feedback. The second commit is much larger, but at its core it only contains the following changes:

Renamed AuthIdentity (parent class of ApiKey) "internalId" attribute to "id"
Renamed Subject "id" and "internalId" attributes to "authenticationId" and "id", respectively
Renamed all methods in InternalAuthorizer to use "id" instead of "internalId"
Renamed SystemInternalId annotation to SystemIdentity

All other changes stem from one of those changes.

One thing I didn't change is that the default table for storing the ID index is still __auth:internal_ids. This was done intentionally to prevent any possible issues in rolling out the new version. Arguably the default could be changed to something more natural like __auth:ids and we could just update our config files to use the old names.

bv-jenkins · 2017-03-28T18:31:28Z

This PR was tested by custom branch build on jenkins

bv-jenkins · 2017-03-28T18:42:39Z

This PR was tested by custom branch build on jenkins

sujithvaddi · 2017-04-19T20:37:33Z

@billkalter Looks good to me. Please go ahead and merge.

verified the changes as listed in the comment above.
checked for any missed references of InternalID, but found none except at a few places where we needed backward compatibility and was intentional.
Emo started fine & all integ tests passed.

billkalter force-pushed the emo-6146 branch from 3d895d0 to 9b69b82 Compare March 14, 2017 19:58

billkalter added 2 commits March 17, 2017 09:11

Reworked API key management to use internal IDs instead of API keys.

998e665

Renamed RoleUpdateRequest to RoleModification for consistency with AP…

f0049d5

…I key management

billkalter force-pushed the emo-6146 branch from 9b69b82 to f0049d5 Compare March 17, 2017 14:16

sujithvaddi reviewed Mar 27, 2017

View reviewed changes

billkalter added 2 commits March 27, 2017 15:51

Partially addressed feedback.

8d852b4

Uniformly renaming API Key "internal ID" as "id"

d2ececf

billkalter merged commit ab63c62 into bazaarvoice:master Apr 20, 2017

billkalter deleted the emo-6146 branch April 20, 2017 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reworked API key management to use internal IDs instead of API keys #99

Reworked API key management to use internal IDs instead of API keys #99

billkalter commented Mar 14, 2017

bv-jenkins commented Mar 14, 2017

bv-jenkins commented Mar 17, 2017

sujithvaddi left a comment

sujithvaddi Mar 27, 2017

sujithvaddi Mar 27, 2017

sujithvaddi Mar 27, 2017

billkalter Mar 27, 2017

sujithvaddi Mar 27, 2017

billkalter commented Mar 28, 2017 •

edited

Loading

bv-jenkins commented Mar 28, 2017

bv-jenkins commented Mar 28, 2017

sujithvaddi commented Apr 19, 2017

Reworked API key management to use internal IDs instead of API keys #99

Reworked API key management to use internal IDs instead of API keys #99

Conversation

billkalter commented Mar 14, 2017

Github Issue

What Are We Doing Here?

How to Test and Verify

Risk

Level

Required Testing

Code Review Checklist

bv-jenkins commented Mar 14, 2017

bv-jenkins commented Mar 17, 2017

sujithvaddi left a comment

Choose a reason for hiding this comment

sujithvaddi Mar 27, 2017

Choose a reason for hiding this comment

sujithvaddi Mar 27, 2017

Choose a reason for hiding this comment

sujithvaddi Mar 27, 2017

Choose a reason for hiding this comment

billkalter Mar 27, 2017

Choose a reason for hiding this comment

sujithvaddi Mar 27, 2017

Choose a reason for hiding this comment

billkalter commented Mar 28, 2017 • edited Loading

bv-jenkins commented Mar 28, 2017

bv-jenkins commented Mar 28, 2017

sujithvaddi commented Apr 19, 2017

billkalter commented Mar 28, 2017 •

edited

Loading