Add basic authentication plugin #1087

yush1ga · 2018-01-19T07:04:46Z

Motivation

Some competitors of Pulsar has the basic authentication plugin.

Modifications

Added the basic authentication plugin.

Result

Pulsar can use basic authentication.

maskit · 2018-01-19T13:50:52Z

pulsar-broker/src/test/resources/authentication/basic/basic.json

@@ -0,0 +1,6 @@
+{
+  "super": {
+    "password": "superPassword",


Seriously? You should google "htpasswd".

Agree, let's use common format for basic user:password file. That would eliminate the needs for a specific script/tool to manage it.

Fixed it and made userId equivalent to role.

merlimat · 2018-01-19T19:39:41Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/web/AuthenticationFilter.java


    public static final String AuthenticatedRoleAttributeName = AuthenticationFilter.class.getName() + "-role";

    public AuthenticationFilter(PulsarService pulsar) {
        this.authenticationService = pulsar.getBrokerService().getAuthenticationService();
+        if (pulsar.getConfiguration().getAuthenticationProviders()
+                .contains(AuthenticationProviderBasic.class.getName())) {
+            isBasic = true;


Rather than a ad-hoc solution, is it possible to have a general way to trigger the right header in the response?

Maybe we can modify AuthenticationService::authenticateHttpRequest. Currently we just ignore exceptions in the method to try all auth providers, but probably we should gather information from the providers. Because "WWW-Authenticate" header can contain multiple challenges, we can suggest all supported authentication types at the first 401 response.
https://tools.ietf.org/html/rfc7235#section-4.1

Also, there is (at least) another response header, "Authentication-Info" (https://tools.ietf.org/html/rfc7615), that will carry information when authentication succeeds. So it would be better to design the API for this mechanism so that providers can respond with arbitrary headers and/or body data regardless of authentication results.

https://tools.ietf.org/html/rfc7235#section-2.1

HTTP does not restrict applications to this simple challenge-response
framework for access authentication. Additional mechanisms can be
used, such as authentication at the transport level or via message
encapsulation, and with additional header fields specifying
authentication information. However, such additional mechanisms are
not defined by this specification.

I think modifying authenticateHttpRequest is not efficient because the information is gathered every time authentication failed.
I fixed AuthentcationFilter class to gather information from providers when it is initialized.

I don’t think we are always be able to gather information on initialization. For example, Digest authentication needs to generate a challenge (nonce) every time.

Make sense.
I added PulsarHttpAuthenticationException class, which has realm information for WWW-Authentication header and the information is gathered in authenticateHttpRequest.

maskit · 2018-01-20T08:41:04Z

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

+    private String authToken;
+
+    public AuthenticationDataBasic(String userId, String password) {
+        authToken = "Basic " + Base64.getEncoder().encodeToString((userId + ":" + password).getBytes());


If I remember correctly, authentication method name is stored in some field of a command. "Basic " is needed for the HTTP header but it's redundant for the pulsar's command.

Also, base64 encoding is needed because of a limitation of HTTP headers. I don't think pulsar's command has the same limitation.

They both increases data length unnecessarily。

I made command data not contain encoded params.

jai1 · 2018-01-22T18:03:33Z

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

+            return password;
+        }
+    }
+}


Missing new line

merlimat · 2018-01-24T06:17:29Z

...oker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationService.java

@@ -81,12 +81,15 @@ public String authenticate(AuthenticationDataSource authData, String authMethodN
        }
    }

-    public String authenticateHttpRequest(HttpServletRequest request) throws AuthenticationException {
+    public String authenticateHttpRequest(HttpServletRequest request) throws PulsarHttpAuthenticationException {
+        PulsarHttpAuthenticationException exception = new PulsarHttpAuthenticationException("Authentication required");


Creating the exception is a very costly operation because it fills up the stack trace, we should only do it in the failure path

I made it create realmInformation string to avoid creating an exception.

rdhabalia · 2018-01-26T07:37:18Z

pulsar-broker/src/test/resources/authentication/basic/.htpasswd

@@ -0,0 +1,2 @@
+superUser:mQQQIsyvvKRtU


I am not sure but should we add this file under license exclude list ?

I added it.

maskit · 2018-01-26T15:37:10Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/web/AuthenticationFilter.java

            HttpServletResponse httpResponse = (HttpServletResponse) response;
+            if (isNotBlank(e.getRealmInformation())) {
+                ((HttpServletResponse) response).setHeader("WWW-Authenticate", e.getRealmInformation());


As I wrote on the previous comment, there is another standard header that is designed for returning information on authentication success, and authentication methods are also allowed to use other non-standard headers. Requiring auth providers to embed the header information into an exception and assuming the header name is "WWW-Authenticate" would be a limitation in the future.

Honestly speaking, I think using an exception to indicate simple authentication failure is a bad design, at least in Java, because of its cost. Authentication failures happen occasionally, and it can be caused easily by attackers. We shouldn't use exceptions for such a expectable case.

@maskit
I roughly agree with you.

using an exception to indicate simple authentication failure is a bad design

I think this should not be fixed in this PR adding authentication plugin.
We should consider good design and make another PR.
That's why, I exclude the code about authentication header from this PR.

maskit

There are few points need to be improved later (e.g. HTTP support, role management), but seems good. Let's revisit them after we improve API.

merlimat

👍

yush1ga added this to the 1.22.0-incubating milestone Jan 19, 2018

yush1ga added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label Jan 19, 2018

yush1ga self-assigned this Jan 19, 2018

yush1ga force-pushed the password-auth branch from b1c0c79 to 1045a8e Compare January 19, 2018 08:02

maskit reviewed Jan 19, 2018

View reviewed changes

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

yush1ga force-pushed the password-auth branch from 448d1b1 to 1a76ca0 Compare January 19, 2018 18:41

merlimat reviewed Jan 19, 2018

View reviewed changes

maskit reviewed Jan 20, 2018

View reviewed changes

jai1 reviewed Jan 22, 2018

View reviewed changes

yush1ga force-pushed the password-auth branch 2 times, most recently from f5b4fec to 4def43d Compare January 24, 2018 01:50

merlimat reviewed Jan 24, 2018

View reviewed changes

yush1ga force-pushed the password-auth branch from b201dae to 601e363 Compare January 26, 2018 01:24

Yuki Shiga added 6 commits January 26, 2018 10:24

Add password authentication plugin

d383909

Use .htpasswd

65dcc2f

Simplify command data

4beca4c

Add PulsarHttpAuthenticationException to get realm information

54b4183

Addressed PR comments

601e363

Fix tests

8c932d9

rdhabalia reviewed Jan 26, 2018

View reviewed changes

maskit reviewed Jan 26, 2018

View reviewed changes

Deleted PulsarHttpAuthenticationException

8dbb49c

yush1ga force-pushed the password-auth branch from a60b83d to 8dbb49c Compare February 2, 2018 02:35

Added .htpasswd to license exclude list

4fddad2

maskit approved these changes Feb 2, 2018

View reviewed changes

merlimat approved these changes Feb 2, 2018

View reviewed changes

merlimat merged commit 71f281a into apache:master Feb 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic authentication plugin #1087

Add basic authentication plugin #1087

yush1ga commented Jan 19, 2018

maskit Jan 19, 2018

merlimat Jan 19, 2018

yush1ga Jan 19, 2018

merlimat Jan 19, 2018

maskit Jan 20, 2018

yush1ga Jan 22, 2018

maskit Jan 22, 2018

yush1ga Jan 24, 2018

maskit Jan 20, 2018

yush1ga Jan 22, 2018

jai1 Jan 22, 2018

yush1ga Jan 24, 2018

merlimat Jan 24, 2018

yush1ga Jan 25, 2018

rdhabalia Jan 26, 2018

yush1ga Feb 2, 2018

maskit Jan 26, 2018

yush1ga Feb 2, 2018

maskit Feb 2, 2018

maskit left a comment

merlimat left a comment

Add basic authentication plugin #1087

Add basic authentication plugin #1087

Conversation

yush1ga commented Jan 19, 2018

Motivation

Modifications

Result

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

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 left a comment

Choose a reason for hiding this comment