PIP-25: Token based authentication #2888

merlimat · 2018-10-30T07:52:03Z

Motivation

This is the first PR for the implementation of token based authentication

Modifications

Added client & broker auth providers
Added CLI tool to manage tokens
Unit tests

TODOs (in separate PRs):

Integration tests
Documentation
C++ (and related) client plugin implementation

...ommon/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java

ivankelly

Some comments about naming so it's clear that private/public can be used.

Also, it would be really good to have an integration test in this PR a) to validate that both the PSK and private/public paths work and b) to illustrate the workflow as a basis for documentation.

...ommon/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java

pom.xml

pulsar-broker/src/main/java/org/apache/pulsar/utils/auth/tokens/TokensCliUtils.java

merlimat · 2018-11-14T08:01:36Z

@ivankelly @maskit Please take another look

merlimat · 2018-11-14T08:03:44Z

Also, it would be really good to have an integration test in this PR a) to validate that both the PSK and private/public paths work

The 2 paths are tested in the auth provider unit test. It's not the same as integration test but it covers all the plugin code, from getting variables from ServerConfiguration to validating the tokens in both cases.

conf/broker.conf

pulsar-broker/src/main/java/org/apache/pulsar/utils/auth/tokens/TokensCliUtils.java

merlimat · 2018-11-15T01:23:29Z

@maskit @ivankelly Updates to support env:, data: and file: as sources for keys and tokens

maskit · 2018-11-15T12:40:46Z

...ommon/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java

+
+        if (validationKeyConfig.startsWith("data:")) {
+            log.info("Reading token validation key from config file");
+            validationKey = validationKeyConfig.substring("data:".length());


This might be valid for a super simple case, but it wouldn’t work for cases that has mime-type or base64 encoding.

Why didn’t you use the URL class?

... because I've never used URL.openStream() before :)

Though, one thing is that the key is expected to be already base64 encoded.

$ bin/pulsar tokens create-secret-key secret-key CBqtuReZe+8nKqtJBbmuPTIg6b4W1KjOS21pcJkTJ/8=

Then one would just have to store the key string somewhere.

I’m ok with parsing the schemes here and having a limitation in its format for now, but I still think this data scheme parsing is incorrect. It always need a comma after the colon.
https://tools.ietf.org/html/rfc2397#section-3

Also, base64 encoding is not mandatory. You can pass URL encoded string instead.

@maskit I have updated to conform with data: standard url, though I had to add manual parsing since java URL class doesn't support it.

Did you look into #1212 I commented on another line? The URL class I suggested is org.apache.pulsar.client.api.url.URL. The parser already exists in it.

merlimat · 2018-11-25T19:15:57Z

@maskit @ivankelly please take another look. I have moved to use standard data: uri and to not force base64 encodings.

maskit · 2018-11-26T10:01:36Z

The "data:" parser you wrote would be a duplicate code because it's already implemented in the in-house URL class I wrote. I'm not going to make a change request because I don't want to delay this feature, but either of the parsers should be removed eventually to eliminate duplicate code.

Also, because environment variables are general enough as data sources, I think "env:" parser should be added as a URLStreamHandler so that other places can use it too via the in-house URL class.

As for "token:", I'm ok with having it in this plugin as a special case. But if you renamed it to "raw:", it could be used on other places.

merlimat · 2018-11-26T19:07:03Z

run integration tests

ivankelly

There's no change here that shows the whole thing working. It doesn't even have to be an integration test, but there should be a test where a client connects to a server using token based auth.

conf/broker.conf

...roker-common/src/main/java/org/apache/pulsar/broker/authentication/utils/AuthTokenUtils.java

...n/src/test/java/org/apache/pulsar/broker/authentication/AuthenticationProviderTokenTest.java

ivankelly · 2018-11-26T20:38:45Z

pulsar-broker/src/main/java/org/apache/pulsar/utils/auth/tokens/TokensCliUtils.java

+        String outputFile;
+
+        @Parameter(names = {
+                "-b", "--base-64" }, description = "Encode the key in base64")


Shouldn't this be enabled by default in the case that -o is not specified?

I thought about that, but don't know which option is more confusing

pulsar-broker/src/main/java/org/apache/pulsar/utils/auth/tokens/TokensCliUtils.java

ivankelly · 2018-11-26T20:49:20Z

pulsar-broker/src/main/java/org/apache/pulsar/utils/auth/tokens/TokensCliUtils.java

+                token = r.readLine();
+            } else if (tokenFile != null) {
+                token = new String(Files.readAllBytes(Paths.get(tokenFile)), Charsets.UTF_8);
+            } else if (System.getenv("TOKEN") != null) {


why is this here? Seems unnecessary. Anyone who wanted to use this would just pass $TOKEN as the parameter on the shell

The main reason is that a "token" if effectively like a password (or a private key): whoever has it, can access the system. Passing that as a CLI argument will expose to other users on the same machine.

and how would they get it into the environment?

In any case, I'm fine with it staying, just not sure how much real security it offers and trying to reduce options.

and how would they get it into the environment?

I guess you can do:

TOKEN=xyz pulsar tokens show

The env variable will not be visible on the process list

pulsar-broker/src/main/java/org/apache/pulsar/utils/auth/tokens/TokensCliUtils.java

pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/AuthenticationTls.java

ivankelly · 2018-11-26T20:54:33Z

pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/AuthenticationToken.java

+    public void configure(String encodedAuthParamString) {
+        // Interpret the whole param string as the token. If the string contains the notation `token:xxxxx` then strip
+        // the prefix
+        if (encodedAuthParamString.startsWith("token:")) {


how is this different to data: on the server side?

Data is meant to carry arbitrary binary data. A token is guaranteed to be a valid ASCII string (being composed of base64 encoded parts).

The encodedAuthParamString are common to all the auth providers and are a way to pass implementation specific configs. In case of token auth, we're taking the "token" string itself.

I didn't want to use data: here to simplify the user input.

It should be all or nothing, no? I don't see reasons to support token, file and env but not data.

no, token instead of data makes sense. token: is used for passing the token to the client. data: is used to allow the server to load the signing key. They're different usecases. The signing key could be quite large in case of public-private key, whereas the token is generally quite small.

Why we shouldn't support data: for small data? Even if it's not a suitable scheme, I don't see reasons to drop it. If we allowed using any schemes for any cases and used the URL class to support them, we wouldn't need to maintain this if-else block.

maskit · 2018-11-26T22:04:17Z

@merlimat

There is just one problem with that class in that it assumes the data will be decoded into a String,

getContent() returns a InputStream so you should be able to read raw data.

Will continue this in a different PR since this will need to change on Athenz plugin too.

Sure, I think I can help you out on this since Pulsar seems like compilable with Java 11 :)

merlimat · 2018-11-27T09:29:10Z

@ivankelly @maskit Addressed most of comments and added simple integration tests, Please one more look

ivankelly

Minor comments, but nothing blocking

pulsar-broker/src/main/java/org/apache/pulsar/utils/auth/tokens/TokensCliUtils.java

ivankelly · 2018-11-27T10:03:03Z

.../java/org/apache/pulsar/tests/integration/auth/token/PulsarTokenAuthenticationBaseSuite.java

+
+        @Cleanup
+        PulsarClient client = PulsarClient.builder()
+                .serviceUrl(pulsarCluster.getPlainTextServiceUrl())


not for this PR, but do we have a requireTLS option? We should have an option to block authorization unless TLS is in use to avoid people sending tokens over cleartext.

I think there was another PR that was meant to allow to disable the clear-text service endpoints.

...test/java/org/apache/pulsar/tests/integration/auth/token/TokenAuthWithPublicPrivateKeys.java

ivankelly · 2018-11-27T10:07:02Z

...roker-common/src/main/java/org/apache/pulsar/broker/authentication/utils/AuthTokenUtils.java

+                return ByteStreams.toByteArray((InputStream) new URL(keyConfUrl).getContent());
+            } catch (Exception e) {
+                throw new IOException(e);
+            }
        } else if (keyConfUrl.startsWith("env:")) {


Couldn't you have a URL handler for this also?

Yes, adding

Actually, there's one tricky part.

The key, passed as an env variable, needs to be base64 encoded. That might not be needed in general case for env: (eg: a token would be fine as it is).

It's kind of confusing and we'd have to define something similar to env:base64,MY_SECRET_KEY_VAR

I think we shoudn’t define encoding field on env scheme to keep it simple. The requirement comes from application use case (which is this plugin here), so the application should take care of it.
If you get URLConnection from the URL object, you can check the scheme name as protocol name.

I didn’t read key generation part, but if the key is in PEM format we can use the same format for all scheme and decode the raw data read from the InputStream in a consistent way, I think.

I have removed the env: schema as it was not working properly. Java seems to be mangling long var values. We can add later when we can make it work.

ivankelly · 2018-11-27T11:11:48Z

When you do the integration tests, we need a test for proxy (it seems to be missing in this and c++ change)

maskit · 2018-11-27T12:24:15Z

pulsar-common/src/main/java/org/apache/pulsar/client/api/url/DataURLStreamHandler.java

-                if (charset == null) {
-                    charset = "US-ASCII";
+                if (contentType == null) {
+                    this.contentType = "application/data";


RFC2397 says:

If <mediatype> is omitted, it defaults to text/plain;charset=US-ASCII.

Suggested change

this.contentType = "application/data";

this.contentType = "text/plain;charset=US-ASCII";

maskit · 2018-11-27T13:12:47Z

pulsar-common/src/main/java/org/apache/pulsar/client/api/url/DataURLStreamHandler.java

                if (matcher.group("base64") == null) {
                    // Support Urlencode but not decode here because already decoded by URI class.
-                    this.data = new String(matcher.group("data").getBytes(), charset);
+                    this.data = matcher.group("data").getBytes();


Well, this is probably equivalent to the original code, so this change is fine.

However, I realized that the both code have the same issue. Because we drop charset information here, users cannot read the data as a string with the specified charset. I think we should stop parsing parameters in mimetype and return the whole mimetype part as content-type.

The problem with original code is that it was getting a list of bytes and converting them into a String (which in case of keys is not possible) and then exposing back to a ByteArrayInputStream. I just cut through the String conversion

Yeah, I can see that, and we shoud fix that part. So this would be out of this PR’s scope but we need to return charset as a part of contentType so that URL class users can convert the data into String after reading the raw data from InputStream. I think we can do this by removing charset part from the regex.

...roker-common/src/main/java/org/apache/pulsar/broker/authentication/utils/AuthTokenUtils.java

maskit

+1 because there's no blockers. I can improve the small stuffs around scheme and URL class later. The behavior in normal use cases wouldn't be changed so much.

pulsar-common/src/main/java/org/apache/pulsar/client/api/url/DataURLStreamHandler.java

merlimat · 2018-11-27T23:15:49Z

When you do the integration tests, we need a test for proxy (it seems to be missing in this and c++ change)

@ivankelly the test is actually going trough the proxy already since pulsarCluster.getPlainTextServiceUrl() returns the proxy container address

merlimat · 2018-11-28T10:05:09Z

@maskit @ivankelly Few more changes:

Fixed integration tests (test is also going against proxy)
Added CLI tools to validate a token against a key
Removed env: as it's not working properly.

ivankelly · 2018-11-28T11:16:47Z

tests/integration/src/test/java/org/apache/pulsar/tests/integration/utils/DockerUtils.java

+        String cmdString = Arrays.stream(cmd).collect(Collectors.joining(" "));
+        ByteBuf stdout = Unpooled.buffer();
+        ByteBuf stderr = Unpooled.buffer();
+        docker.execStartCmd(execid).withDetach(false)


There's a lot of code in common with runCommand here. Doesn't need to be in this patch, but it should be factors out to something that takes a Consumer<byte[]>

merlimat · 2018-11-28T19:49:14Z

run cpp tests

PIP-25: Token based authentication

0a519d6

merlimat added the type/feature The PR added a new feature or issue requested a new feature label Oct 30, 2018

merlimat added this to the 2.3.0 milestone Oct 30, 2018

merlimat self-assigned this Oct 30, 2018

merlimat requested review from ivankelly, sijie, rdhabalia, jerrypeng, jiazhai and nkurihar October 30, 2018 07:52

maskit reviewed Oct 30, 2018

View reviewed changes

...ommon/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java Outdated Show resolved Hide resolved

ivankelly requested changes Oct 30, 2018

View reviewed changes

...ommon/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java Show resolved Hide resolved

pom.xml Show resolved Hide resolved

pulsar-broker/src/main/java/org/apache/pulsar/utils/auth/tokens/TokensCliUtils.java Outdated Show resolved Hide resolved

merlimat added 3 commits November 13, 2018 10:46

Merge remote-tracking branch 'apache/master' into jwt

77011c0

Addressed comments

43ab2a7

Use Authorization header

3f01509

maskit reviewed Nov 14, 2018

View reviewed changes

conf/broker.conf Outdated Show resolved Hide resolved

maskit reviewed Nov 14, 2018

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/utils/auth/tokens/TokensCliUtils.java Outdated Show resolved Hide resolved

merlimat added 2 commits November 14, 2018 17:20

Update to support env: data: and file: as sources for keys and tokens

01d55a3

Fixed cli description

a46916e

Updated broker.conf

0760e68

maskit reviewed Nov 15, 2018

View reviewed changes

merlimat added 3 commits November 21, 2018 18:12

Merge remote-tracking branch 'apache/master' into jwt

6f9b1a1

Improved consistency in reading keys and CLI tools

a479e8e

Merge remote-tracking branch 'apache/master' into jwt

2fed618

Merge branch 'master' into jwt

5908407

Merge remote-tracking branch 'apache/master' into jwt

0004810

ivankelly requested changes Nov 26, 2018

View reviewed changes

merlimat added 2 commits November 26, 2018 22:25

Accept rel time with no specified unit

a2dc91f

Fixed reading data: URL

811643a

merlimat force-pushed the jwt branch from f48edab to 811643a Compare November 27, 2018 07:02

merlimat added 3 commits November 26, 2018 23:46

Addressed comments

5deae68

Added integration tests

a30efb7

Merge remote-tracking branch 'apache/master' into jwt

a6856db

merlimat mentioned this pull request Nov 27, 2018

PIP-25: C++ / Python / Go implementation for token authentication #3067

Merged

ivankelly approved these changes Nov 27, 2018

View reviewed changes

maskit reviewed Nov 27, 2018

View reviewed changes

...roker-common/src/main/java/org/apache/pulsar/broker/authentication/utils/AuthTokenUtils.java Show resolved Hide resolved

maskit approved these changes Nov 27, 2018

View reviewed changes

maskit reviewed Nov 27, 2018

View reviewed changes

pulsar-common/src/main/java/org/apache/pulsar/client/api/url/DataURLStreamHandler.java Outdated Show resolved Hide resolved

Addressed comments

2cddaa8

merlimat added 3 commits November 28, 2018 01:57

Added CLI command to validate token against key

34b70be

Fixed integration tests

ae8a52d

Removed env:

703d5ca

ivankelly approved these changes Nov 28, 2018

View reviewed changes

Fixed rel time parsing

3744510

merlimat merged commit a99f733 into apache:master Nov 28, 2018

merlimat deleted the jwt branch November 28, 2018 20:42

	this.contentType = "application/data";
	this.contentType = "text/plain;charset=US-ASCII";

PIP-25: Token based authentication #2888

PIP-25: Token based authentication #2888

Conversation

merlimat commented Oct 30, 2018 • edited Loading

Motivation

Modifications

ivankelly left a comment

Choose a reason for hiding this comment

merlimat commented Nov 14, 2018

merlimat commented Nov 14, 2018

merlimat commented Nov 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merlimat commented Nov 25, 2018

maskit commented Nov 26, 2018 • edited Loading

merlimat commented Nov 26, 2018

ivankelly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maskit commented Nov 26, 2018

merlimat commented Nov 27, 2018

ivankelly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivankelly commented Nov 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maskit left a comment

Choose a reason for hiding this comment

merlimat commented Nov 27, 2018

merlimat commented Nov 28, 2018

Choose a reason for hiding this comment

merlimat commented Nov 28, 2018

merlimat commented Oct 30, 2018 •

edited

Loading

maskit commented Nov 26, 2018 •

edited

Loading