Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PIP-25: Token based authentication #2888

Merged
merged 23 commits into from
Nov 28, 2018
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions conf/broker.conf
Original file line number Diff line number Diff line change
Expand Up @@ -285,18 +285,18 @@ anonymousUserRole=

### --- Token Authentication Provider --- ###
merlimat marked this conversation as resolved.
Show resolved Hide resolved

## Simmetric key
## Symmetric key
# Configure the secret key to be used to validate auth tokens
# The key can be specified like:
# tokenSecretKey=data:xxxxxxxxx
# tokenSecretKey=data:base64,xxxxxxxxx
# tokenSecretKey=file:///my/secret.key
# tokenSecretKey=env:MY_SECRET_KEY_VAR
tokenSecretKey=

## Asymmetric public/private key pair
# Configure the public key to be used to validate auth tokens
# The key can be specified like:
# tokenPublicKey=data:xxxxxxxxx
# tokenPublicKey=data:base64,xxxxxxxxx
# tokenPublicKey=file:///my/public.key
# tokenPublicKey=env:MY_PUBLIC_KEY_VAR
tokenPublicKey=
Expand Down
18 changes: 18 additions & 0 deletions conf/proxy.conf
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,24 @@ tlsHostnameVerificationEnabled=false
# certificate isn't trusted.
tlsRequireTrustedClientCertOnConnect=false

### --- Token Authentication Provider --- ###

## Symmetric key
# Configure the secret key to be used to validate auth tokens
# The key can be specified like:
# tokenSecretKey=data:base64,xxxxxxxxx
# tokenSecretKey=file:///my/secret.key
# tokenSecretKey=env:MY_SECRET_KEY_VAR
tokenSecretKey=

## Asymmetric public/private key pair
# Configure the public key to be used to validate auth tokens
# The key can be specified like:
# tokenPublicKey=data:base64,xxxxxxxxx
# tokenPublicKey=file:///my/public.key
# tokenPublicKey=env:MY_PUBLIC_KEY_VAR
tokenPublicKey=


### --- Deprecated config variables --- ###

Expand Down
6 changes: 6 additions & 0 deletions pulsar-broker-common/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,12 @@
<artifactId>pulsar-zookeeper-utils</artifactId>
<version>${project.version}</version>
</dependency>

<dependency>
<groupId>${project.groupId}</groupId>
<artifactId>pulsar-common</artifactId>
<version>${project.version}</version>
</dependency>

<dependency>
<groupId>com.google.guava</groupId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
*/
package org.apache.pulsar.broker.authentication.utils;

import com.google.common.io.ByteStreams;

import io.jsonwebtoken.JwtBuilder;
import io.jsonwebtoken.Jwts;
import io.jsonwebtoken.SignatureAlgorithm;
Expand All @@ -26,9 +28,7 @@
import io.jsonwebtoken.security.Keys;

import java.io.IOException;
import java.net.URI;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.io.InputStream;
import java.security.Key;
import java.security.KeyFactory;
import java.security.PrivateKey;
Expand All @@ -42,6 +42,8 @@

import lombok.experimental.UtilityClass;

import org.apache.pulsar.client.api.url.URL;

@UtilityClass
public class AuthTokenUtils {

Expand Down Expand Up @@ -90,39 +92,18 @@ public static String createToken(Key signingKey, String subject, Optional<Date>
}

public static byte[] readKeyFromUrl(String keyConfUrl) throws IOException {
if (keyConfUrl.startsWith("data:")) {
return readDataUrl(keyConfUrl);
if (keyConfUrl.startsWith("data:") || keyConfUrl.startsWith("file:")) {
try {
return ByteStreams.toByteArray((InputStream) new URL(keyConfUrl).getContent());
} catch (Exception e) {
throw new IOException(e);
}
} else if (keyConfUrl.startsWith("env:")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, adding

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

String envVarName = keyConfUrl.substring("env:".length());
return Decoders.BASE64.decode(System.getenv(envVarName));
} else if (keyConfUrl.startsWith("file:")) {
URI filePath = URI.create(keyConfUrl);
return Files.readAllBytes(Paths.get(filePath));
} else {
merlimat marked this conversation as resolved.
Show resolved Hide resolved
// Assume the key content was passed in base64
return Decoders.BASE64.decode(keyConfUrl);
}
}

private static byte[] readDataUrl(String data) throws IOException {
// Expected format is:
// data:[<mediatype>][;base64],<data>

// Ignore mediatype and only support base64 encoding
String[] parts = data.split(",", 2);
String header = parts[0];
String encodedData = parts[1];

if (header.contains(";")) {
String encodingType = header.split(";")[1];
if (!"base64".equals(encodingType)) {
throw new IOException("Data url encoding not supported: " + encodingType);
}

return Decoders.BASE64.decode(encodedData);
} else {
// No encoding specified
return encodedData.getBytes();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public static class CommandCreateSecretKey {
String outputFile;

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

public void run() throws IOException {
Expand Down Expand Up @@ -115,22 +115,32 @@ public static class CommandCreateToken {
"--expiry-time" }, description = "Relative expiry time for the token (eg: 1h, 3d, 10y). (m=minutes) Default: no expiration")
private String expiryTime;

@Parameter(names = { "-pk",
"--is-private-key" }, description = "Indicate the signing key is a private key (rather than a symmetric secret key)")
private Boolean isPrivateKey = false;
@Parameter(names = { "-sk",
"--secret-key" }, description = "Pass the secret key for signing the token. This can either be: data:, file:, etc..")
private String secretKey;

@Parameter(names = { "-k",
"--signing-key" }, description = "Pass the signing key. This can either be: data:, file:, etc..", required = true)
private String key;
@Parameter(names = { "-pk",
"--private-key" }, description = "Pass the private key for signing the token. This can either be: data:, file:, etc..")
private String privateKey;

public void run() throws Exception {
byte[] encodedKey = AuthTokenUtils.readKeyFromUrl(key);
if (secretKey == null && privateKey == null) {
System.err.println(
"Either --secret-key or --private-key needs to be passed for signing a token");
System.exit(1);
} else if (secretKey != null && privateKey != null) {
System.err.println(
"Only one between --secret-key and --private-key needs to be passed for signing a token");
merlimat marked this conversation as resolved.
Show resolved Hide resolved
System.exit(1);
}

Key signingKey;

if (isPrivateKey) {
if (privateKey != null) {
byte[] encodedKey = AuthTokenUtils.readKeyFromUrl(privateKey);
signingKey = AuthTokenUtils.decodePrivateKey(encodedKey);
} else {
byte[] encodedKey = AuthTokenUtils.readKeyFromUrl(secretKey);
signingKey = AuthTokenUtils.decodeSecretKey(encodedKey);
}

Expand All @@ -157,7 +167,7 @@ public static class CommandShowToken {
private Boolean stdin = false;

@Parameter(names = { "-f",
"--token-file" }, description = "Read tokn from a file")
"--token-file" }, description = "Read token from a file")
private String tokenFile;

public void run() throws Exception {
Expand All @@ -174,7 +184,7 @@ public void run() throws Exception {
token = System.getenv("TOKEN");
} else {
System.err.println(
"Token needs to be either passed through `--stdin`, `--token-file` or by `TOKEN` environment variable");
"Token needs to be either passed as an argument or through `--stdin`, `--token-file` or by `TOKEN` environment variable");
merlimat marked this conversation as resolved.
Show resolved Hide resolved
System.exit(1);
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,15 @@

public class DataURLStreamHandler extends URLStreamHandler {

class DataURLConnection extends URLConnection {
static class DataURLConnection extends URLConnection {
private boolean parsed = false;
private String contentType;
private String data;
private byte[] data;
private URI uri;

private static final Pattern pattern = Pattern.compile(
"(?<mimeType>[^;,]+)?(;(?<charset>charset=[^;,]+))?(;(?<base64>base64))?,(?<data>.+)", Pattern.DOTALL);

protected DataURLConnection(URL url) {
super(url);
try {
Expand All @@ -57,20 +60,23 @@ public void connect() throws IOException {
if (this.uri == null) {
throw new IOException();
}
Pattern pattern = Pattern.compile(
"(?<mimeType>.+?)(;(?<charset>charset=.+?))?(;(?<base64>base64?))?,(?<data>.+)", Pattern.DOTALL);

Matcher matcher = pattern.matcher(this.uri.getSchemeSpecificPart());
if (matcher.matches()) {
this.contentType = matcher.group("mimeType");
String charset = matcher.group("charset");
if (charset == null) {
charset = "US-ASCII";
if (contentType == null) {
this.contentType = "application/data";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RFC2397 says:

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

for (int i =0; i < matcher.groupCount(); i++) {
System.out.println("Group: " + matcher.group(i));
merlimat marked this conversation as resolved.
Show resolved Hide resolved
}

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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
this.data = new String(Base64.getDecoder().decode(matcher.group("data")), charset);
this.data = Base64.getDecoder().decode(matcher.group("data"));
}
} else {
throw new MalformedURLException();
Expand All @@ -83,7 +89,7 @@ public long getContentLengthLong() {
long length;
try {
this.connect();
length = this.data.length();
length = this.data.length;
} catch (IOException e) {
length = -1;
}
Expand All @@ -109,7 +115,7 @@ public String getContentEncoding() {

public InputStream getInputStream() throws IOException {
this.connect();
return new ByteArrayInputStream(this.data.getBytes());
return new ByteArrayInputStream(this.data);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
import java.util.Map;

public class PulsarURLStreamHandlerFactory implements URLStreamHandlerFactory {
static Map<String, Class<? extends URLStreamHandler>> handlers;
private static final Map<String, Class<? extends URLStreamHandler>> handlers;
static {
handlers = new HashMap<>();
handlers.put("data", DataURLStreamHandler.class);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@
import java.net.URLStreamHandlerFactory;

public class URL {
private static URLStreamHandlerFactory urlStreamHandlerFactory = new PulsarURLStreamHandlerFactory();
private java.net.URL url;
private static final URLStreamHandlerFactory urlStreamHandlerFactory = new PulsarURLStreamHandlerFactory();
private final java.net.URL url;

public URL(String spec)
throws MalformedURLException, URISyntaxException, InstantiationException, IllegalAccessException {
Expand All @@ -47,7 +47,7 @@ public Object getContent() throws IOException {
return this.url.getContent();
}

public Object getContent(Class[] classes) throws IOException {
public Object getContent(Class<?>[] classes) throws IOException {
return this.url.getContent(classes);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,21 @@ public static long parseRelativeTimeInSeconds(String relativeTime) {
throw new IllegalArgumentException("exipiry time cannot be empty");
}

char lastChar = relativeTime.charAt(relativeTime.length() - 1);
int lastIndex= relativeTime.length() - 1;
char lastChar = relativeTime.charAt(lastIndex);
final char timeUnit;

if (!Character.isAlphabetic(lastChar)) {
throw new IllegalArgumentException("Relative time should contain time unit. eg: 3h or 5d");
// No unit specified, assume seconds
timeUnit = 's';
lastIndex = relativeTime.length();
} else {
timeUnit = Character.toLowerCase(lastChar);
}

long duration = Long.parseLong(relativeTime.substring(0, relativeTime.length() - 1));
long duration = Long.parseLong(relativeTime.substring(0, lastIndex));

switch (lastChar) {
switch (timeUnit) {
case 's':
return duration;
case 'm':
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,19 @@
public class RelativeTimeUtilTest {
@Test
public void testParseRelativeTime() {
assertEquals(RelativeTimeUtil.parseRelativeTimeInSeconds("-1"), -1);
assertEquals(RelativeTimeUtil.parseRelativeTimeInSeconds("7"), 7);
assertEquals(RelativeTimeUtil.parseRelativeTimeInSeconds("3s"), 3);
assertEquals(RelativeTimeUtil.parseRelativeTimeInSeconds("3S"), 3);
assertEquals(RelativeTimeUtil.parseRelativeTimeInSeconds("5m"), TimeUnit.MINUTES.toSeconds(5));
assertEquals(RelativeTimeUtil.parseRelativeTimeInSeconds("5M"), TimeUnit.MINUTES.toSeconds(5));
assertEquals(RelativeTimeUtil.parseRelativeTimeInSeconds("7h"), TimeUnit.HOURS.toSeconds(7));
assertEquals(RelativeTimeUtil.parseRelativeTimeInSeconds("7H"), TimeUnit.HOURS.toSeconds(7));
assertEquals(RelativeTimeUtil.parseRelativeTimeInSeconds("9d"), TimeUnit.DAYS.toSeconds(9));
assertEquals(RelativeTimeUtil.parseRelativeTimeInSeconds("9D"), TimeUnit.DAYS.toSeconds(9));
assertEquals(RelativeTimeUtil.parseRelativeTimeInSeconds("3w"), 7 * TimeUnit.DAYS.toSeconds(3));
assertEquals(RelativeTimeUtil.parseRelativeTimeInSeconds("11y"), 365 * TimeUnit.DAYS.toSeconds(11));
assertEquals(RelativeTimeUtil.parseRelativeTimeInSeconds("11Y"), 365 * TimeUnit.DAYS.toSeconds(11));

// Negative interval
assertEquals(RelativeTimeUtil.parseRelativeTimeInSeconds("-5m"), -TimeUnit.MINUTES.toSeconds(5));
Expand All @@ -44,14 +52,6 @@ public void testParseRelativeTime() {
// expected
}

try {
// No time unit specified
RelativeTimeUtil.parseRelativeTimeInSeconds("1234");
fail("should have failed");
} catch (IllegalArgumentException e) {
// expected
}

try {
// Invalid time unit specified
RelativeTimeUtil.parseRelativeTimeInSeconds("1234x");
Expand Down
Loading