[GitHub] tinkerpop pull request #534: TINKERPOP-1566 Kerberos authentication for grem...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/tinkerpop/pull/534


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tinkerpop pull request #534: TINKERPOP-1566 Kerberos authentication for grem...

[vtslab]

Github user vtslab commented on a diff in the pull request:

https://github.com/apache/tinkerpop/pull/534#discussion_r102313638
  
--- Diff: 
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpBasicAuthenticationHandler.java
 ---
@@ -92,6 +102,13 @@ public void channelRead(final ChannelHandlerContext 
ctx, final Object msg) {
 try {
 authenticator.authenticate(credentials);
 ctx.fireChannelRead(request);
+
+// User name logged with the remote socket address and 
authenticator classname for audit logging
+if (authenticationSettings.enableAuditLog) {
+String[] authClassParts = 
authenticator.getClass().toString().split("[.]");
+auditLogger.info("User {} with address {} 
authenticated by {}", credentials.get(PROPERTY_USERNAME),
+
ctx.channel().remoteAddress().toString().substring(1), 
authClassParts[authClassParts.length - 1]);
--- End diff --

Thanks for clarifying, I will correct it. Tests are still running.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tinkerpop pull request #534: TINKERPOP-1566 Kerberos authentication for grem...

[robertdale]

Github user robertdale commented on a diff in the pull request:

https://github.com/apache/tinkerpop/pull/534#discussion_r102104550
  
--- Diff: docs/src/reference/gremlin-applications.asciidoc ---
@@ -1035,6 +1035,7 @@ The following table describes the various YAML 
configuration options that Gremli
 |=
 |Key |Description |Default
 |authentication.className |The fully qualified classname of an 
`Authenticator` implementation to use.  If this setting is not present, then 
authentication is effectively disabled. |`AllowAllAuthenticator`
+|authentication.enableAuditLog |The available authenticators can issue 
audit logging messages, binding the authenticated user to his remote socket 
address and binding requests with a gremlin query to the remote socket address. 
For privacy reasons, the default value of this setting is false. The audit 
logging messages are logged at the INFO level via the 
`audit.org.apache.tinkerpop.gremlin.server` logger, which can be configured 
using the log4j.properties file. |false
--- End diff --

What I meant is that this one was not in alphabetical order and perhaps it 
should be.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tinkerpop pull request #534: TINKERPOP-1566 Kerberos authentication for grem...

[robertdale]

Github user robertdale commented on a diff in the pull request:

https://github.com/apache/tinkerpop/pull/534#discussion_r102104360
  
--- Diff: 
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpBasicAuthenticationHandler.java
 ---
@@ -92,6 +102,13 @@ public void channelRead(final ChannelHandlerContext 
ctx, final Object msg) {
 try {
 authenticator.authenticate(credentials);
 ctx.fireChannelRead(request);
+
+// User name logged with the remote socket address and 
authenticator classname for audit logging
+if (authenticationSettings.enableAuditLog) {
+String[] authClassParts = 
authenticator.getClass().toString().split("[.]");
+auditLogger.info("User {} with address {} 
authenticated by {}", credentials.get(PROPERTY_USERNAME),
+
ctx.channel().remoteAddress().toString().substring(1), 
authClassParts[authClassParts.length - 1]);
--- End diff --

Let me elaborate.  substring(1) assumes the toString() always starts with 
'/'.  However, if the hostname were resolved, then it would be in the format of 
"hostname/IP address:port".   substring(1) would result in "ostname/IP 
address:port".   It might not ever happen, but wanted to point it out for 
awareness.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tinkerpop pull request #534: TINKERPOP-1566 Kerberos authentication for grem...

[vtslab]

Github user vtslab commented on a diff in the pull request:

https://github.com/apache/tinkerpop/pull/534#discussion_r102095814
  
--- Diff: docs/src/reference/gremlin-applications.asciidoc ---
@@ -1035,6 +1035,7 @@ The following table describes the various YAML 
configuration options that Gremli
 |=
 |Key |Description |Default
 |authentication.className |The fully qualified classname of an 
`Authenticator` implementation to use.  If this setting is not present, then 
authentication is effectively disabled. |`AllowAllAuthenticator`
+|authentication.enableAuditLog |The available authenticators can issue 
audit logging messages, binding the authenticated user to his remote socket 
address and binding requests with a gremlin query to the remote socket address. 
For privacy reasons, the default value of this setting is false. The audit 
logging messages are logged at the INFO level via the 
`audit.org.apache.tinkerpop.gremlin.server` logger, which can be configured 
using the log4j.properties file. |false
--- End diff --

Answer to this riddle (needed some thought on my part): also other config 
items, like scriptEngines..config, are at the bottom of their section. 
Just because it looks more orderly in the config file.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tinkerpop pull request #534: TINKERPOP-1566 Kerberos authentication for grem...

[robertdale]

Github user robertdale commented on a diff in the pull request:

https://github.com/apache/tinkerpop/pull/534#discussion_r102062737
  
--- Diff: 
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpBasicAuthenticationHandler.java
 ---
@@ -92,6 +102,13 @@ public void channelRead(final ChannelHandlerContext 
ctx, final Object msg) {
 try {
 authenticator.authenticate(credentials);
 ctx.fireChannelRead(request);
+
+// User name logged with the remote socket address and 
authenticator classname for audit logging
+if (authenticationSettings.enableAuditLog) {
+String[] authClassParts = 
authenticator.getClass().toString().split("[.]");
+auditLogger.info("User {} with address {} 
authenticated by {}", credentials.get(PROPERTY_USERNAME),
+
ctx.channel().remoteAddress().toString().substring(1), 
authClassParts[authClassParts.length - 1]);
--- End diff --

substring(1) assumes that the remoteAddress always has an unresolved 
(reverse lookup) hostname. I don't know if this is always the case.




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tinkerpop pull request #534: TINKERPOP-1566 Kerberos authentication for grem...

[robertdale]

Github user robertdale commented on a diff in the pull request:

https://github.com/apache/tinkerpop/pull/534#discussion_r102062848
  
--- Diff: 
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/SaslAuthenticationHandler.java
 ---
@@ -94,13 +99,17 @@ public void channelRead(final ChannelHandlerContext 
ctx, final Object msg) throw
 ctx.writeAndFlush(error);
 return;
 }
-
+
 try {
 final byte[] saslMessage = 
negotiator.get().evaluateResponse(saslResponse);
 if (negotiator.get().isComplete()) {
-// todo: do something with this user
 final AuthenticatedUser user = 
negotiator.get().getAuthenticatedUser();
-
+// User name logged with the remote socket 
address and authenticator classname for audit logging
+if (authenticationSettings.enableAuditLog) {
+String[] authClassParts = 
authenticator.getClass().toString().split("[.]");
+auditLogger.info("User {} with address {} 
authenticated by {}", user.getName(),
+
ctx.channel().remoteAddress().toString().substring(1), 
authClassParts[authClassParts.length - 1]);
--- End diff --

substring(1) again


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tinkerpop pull request #534: TINKERPOP-1566 Kerberos authentication for grem...

[robertdale]

Github user robertdale commented on a diff in the pull request:

https://github.com/apache/tinkerpop/pull/534#discussion_r102058441
  
--- Diff: docs/src/reference/gremlin-applications.asciidoc ---
@@ -1035,6 +1035,7 @@ The following table describes the various YAML 
configuration options that Gremli
 |=
 |Key |Description |Default
 |authentication.className |The fully qualified classname of an 
`Authenticator` implementation to use.  If this setting is not present, then 
authentication is effectively disabled. |`AllowAllAuthenticator`
+|authentication.enableAuditLog |The available authenticators can issue 
audit logging messages, binding the authenticated user to his remote socket 
address and binding requests with a gremlin query to the remote socket address. 
For privacy reasons, the default value of this setting is false. The audit 
logging messages are logged at the INFO level via the 
`audit.org.apache.tinkerpop.gremlin.server` logger, which can be configured 
using the log4j.properties file. |false
--- End diff --

Should this be in alphabetical order?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tinkerpop pull request #534: Tinkerpop 1566 Kerberos

[spmallette]

Github user spmallette commented on a diff in the pull request:

https://github.com/apache/tinkerpop/pull/534#discussion_r96403984
  
--- Diff: 
gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ser/AbstractGryoMessageSerializerV1d0.java
 ---
@@ -301,9 +301,12 @@ private Object serializeResultToString(final 
ResponseMessage msg) {
 if (msg.getResult().getData() == null) return "null";
 
 // the IteratorHandler should return a collection so keep it as 
such
+// Sasl authentication needs byte[] to pass unchanged
 final Object o = msg.getResult().getData();
 if (o instanceof Collection) {
 return ((Collection) o).stream().map(d -> null == d ? "null" : 
d.toString()).collect(Collectors.toList());
+} else if (o instanceof byte[]) {
--- End diff --

I think you should undo this special handling - based on my explanation on 
#533 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] tinkerpop pull request #534: Tinkerpop 1566

[vtslab]

GitHub user vtslab opened a pull request:

https://github.com/apache/tinkerpop/pull/534

Tinkerpop 1566

This PR includes three items (as stated in the changelog):
  1 Added Kerberos authentication to `gremlin-server` for websockets and 
nio transport.
  2 Added audit logging of authenticated users and of gremlin queries to 
`gremlin-server`.
  3 Fixed `gremlin-driver`'s support for string results regarding returned 
byte arrays 
from `Sasl` authentication.

Regarding item 1, I did not attempt to provide Kerberos authentication for 
http 
transport, as I assumed http will not be very popular anymore, now that the 
GLV's
are available for accessing graphs via gremlin-server.

Item 2, audit logging, naturally belongs to Kerberos authentication. 
Kerberos is
important in providing access to confidential data, that is, being sure of
someone's identity without having him logging in for each service. Some 
confidential data, like personal data, often have legal obligations 
regarding 
logging of their access: that is what item 2 provides.

Item 3 is just a minor issue that surfaced during test development of 
Kerberos 
authentication.

An ample number of integration tests is provided. In addition, I did manual 
tests
in a working freeIPA Kerberos environment to verify the proper working. 

Reviewers wanting a short reminder of Kerberos authenticationb are referred 
to:
http://www.roguelynn.com/words/explain-like-im-5-kerberos/
[It learnt me a lot, I am not trying to be arrogant :-)]

The main design choices I made are:

i) Krb5Authenticator does not refer to policy servers or storage backends 
for 
authorization, but rather assumes that any user who can be authenticated 
using
Kerberos, is also authorized to access the service. Others could extend on 
this.

ii) The JAAS entry for Krb5Authenticator was not made configurable, apart 
from 
the principal name and keytab location to be provided in the yaml file. 
Using 
a separate JAAS config file would primarily introduce more flexibility in 
getting 
the config wrong. This choice is in line with the current situation with all
authenticator configuration in gremlin-server's yaml file. But the choice 
is 
not consistent with other Apache projects like Hadoop and HBase.

iii) Audit logging was made into a general feature that also works for 
other 
authenticators. It has to be explicitly enabled, though, with a property in 
gremlin-server's yaml file, because the audit logs can contain confidential 
data.

iv) Audit logging was given a separate logger apart from the 
org.apache.tinkerpop
naming tree, so that its level can be set to INFO without influencing level
settings of the normal logging. The logger name, 
"audit.org.apache.tinkerpop.gremlin.server",
was defined in GremlinServer, for lack of a better location.

v) Apache Kerby was used as the Kerberos Key Distribution Center (KDC) for 
the 
Kerberos integration tests, because it also belongs to Apache and proved 
easy to 
use. The project is still in RC2 status, though, but is only a test 
dependency.

Finally, running integration test on gremlin-server still results in two 
errors,
probably due to the presence of the gremlinjaas.conf file in the test 
resources.
I did not correct these, because I was not sure whether it could be due to 
my
test environment.

Failed tests: 
  
GremlinServerAuthIntegrateTest.shouldFailAuthenticateWithPlainTextNoCredentials:130
 
expected: 
but was:
  
GremlinServerAuthOldIntegrateTest.shouldFailAuthenticateWithPlainTextNoCredentials:133
 
expected: 
but was:


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/vtslab/incubator-tinkerpop TINKERPOP-1566

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/tinkerpop/pull/534.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #534


commit f09546e8bf61d0fc5ab5d92568ee9e7b6e86773e
Author: vtslab 
Date:   2016-11-25T10:48:32Z

Kerberos authenticator files added

commit 0f43649f86fcf049a5d2749387f832d2ed71fa9f
Author: vtslab 
Date:   2016-11-28T12:16:51Z

Added failing test shouldAuthenticateWithSerializeResultToString

commit d874207fb2f53139cc131012a866b3c271a0f73f
Author: HadoopMarc 
Date:   2016-12-03T15:41:47Z

Fixed problem with non-lowercase hostname

commit debb7c854a9a3042f091f5751182b2f563151f1e
Author: HadoopMarc 
Date:   2016-12-04T15:14:05Z

Added Kerberos tests for client

commit cb81fcfbc968c09bee4ac4ab059adf4782df037d
Author: HadoopMarc 
Date: