Example Authentication providers #3410

niloc132 · 2023-02-06T20:14:16Z

Remaining to mark it as ready:

better artifact IDs for the example providers
documentation for the oidc provider
docker-compose samples that include DH itself (sub point, confirm that shaded jars work as expected)
psk html sample

Discussion points for reviews:

Do we publish these at all? that is, JAVA_PUBLIC vs JAVA_BASIC?
Logging standards for each provider, how noisy is too noisy? do we always emit a clickable link? do we always log what auth providers are on?
authType strings - okay to default to the classname for dh-provided ones, easier to user from a client (rather than guessing from the AuthHandlers value
futher parameterize the oidc provider to actually be generic for any oidc provider (this implies testing with other providers), or just be explicit that this is just a 'keycloak' provider?

Fixes #2564
Fixes #2934

Partial deephaven#2934

formed status objects

...cket-jakarta/src/main/java/io/grpc/servlet/web/websocket/MultiplexedWebsocketStreamImpl.java

devinrsmith

I think these are mostly great. I know we've talked a little bit about the packaging changes you expect to make.

I glossed over oidc and postrges impls and JS.

I have successfully run the JS examples, as well as extended the java client to more thoroughly test psk, mtls, and anonymous.

devinrsmith · 2023-02-09T15:30:13Z

...providers/mtls/src/main/java/io/deephaven/authentication/mtls/MTlsAuthenticationHandler.java

+            return Optional.of(new AuthContext() {
+                @Override
+                public LogOutput append(LogOutput logOutput) {
+                    return logOutput.append(x509Certificates.get(0).getSubjectDN().getName());
+                }
+            });


I think we want to move towards more structured AuthContexts that capture the full state:

class MtlsAuthContext implements AuthContext { private final List<X509Certificate> ... ... }

I know we don't have great Authorization tie-ins at the moment, but mTLS in particular is a cool case b/c multiple privileges can be attached to a single certificate.

Definitely agreed, but I'd like to tackle that later, and offer some base implementations along the way, with things like roles optionally populated. As it stands today, every implementation of AuthContext is functionally the same - there is no difference between Anonymous and SuperUser except what they log.

Users have to implement their own versions of AuthWiring classes. I think Devin is asking for the ability to class-cast and invoke an accessor method. I can't see a good reason to not include that at this time. If a user does indeed subclass, this is a simple java-ism that they will want to be able to do.

Only that I want to separate the goals here, focusing on the authentication side, making sure it works with several use cases, and follow up with authorization, as Auth(oriziation)Wiring is explicitly the second. I think we need more than "provide some subclasses and cast" to get that to make sense.

devinrsmith · 2023-02-09T15:30:57Z

...providers/mtls/src/main/java/io/deephaven/authentication/mtls/MTlsAuthenticationHandler.java

+
+    @Override
+    public void initialize(String targetUrl) {
+


Probably worth a log.

Yep see description, I'm hoping for some feedback on expected conventions - how noisy, do we log at all, just the URL, do we really need every handler to log roughly the same URL, etc

targetUrl is a bit weird to pass into this function. I understand we're giving psk a chance to log out a "pretty" URL, but maybe there is a better way to structure this.

My vote might be to rip out the init logging (except for Anonymous), and then have the server print targetUrl once.

devinrsmith · 2023-02-09T15:34:42Z

authentication/example-providers/sql-username-password/init-users.sql

+);
+
+INSERT INTO deephaven_username_password_auth.users (username, password_hash, rounds)
+VALUES ('admin', '$2a$10$kjbt1Fq4k4W6EB67GDhAauuIWeI8ppx2gsi6.zLL2R5UYokek8nqO', 10);


It looks like rounds is encoded in the password_hash; rounds may be extraneous?

Yes, unfortunately I would have to implement my own parsing, the existing bcrypt java library doesn't offer a way to read that value, so I just stored it as a separate column.

Looks like https://github.com/patrickfav/bcrypt might be a more recently maintained alternative? Quick look at the API shows that it supports parsing w/o explicitly passing in rounds and improvements https://github.com/patrickfav/bcrypt#enhancements-over-jbcrypt

Okay to have as an optional, very-backlogged followup?

If we're doing this "actually 100% right" there is a lot more that needs to be done too, like pooling jdbc connections, explicitly supporting other drivers, etc.

web/client-api/src/main/java/io/deephaven/web/public/authentication/basic.html

nbauernfeind · 2023-02-14T16:25:05Z

...providers/mtls/src/main/java/io/deephaven/authentication/mtls/MTlsAuthenticationHandler.java

+            return Optional.of(new AuthContext() {
+                @Override
+                public LogOutput append(LogOutput logOutput) {
+                    return logOutput.append(x509Certificates.get(0).getSubjectDN().getName());
+                }
+            });


Users have to implement their own versions of AuthWiring classes. I think Devin is asking for the ability to class-cast and invoke an accessor method. I can't see a good reason to not include that at this time. If a user does indeed subclass, this is a simple java-ism that they will want to be able to do.

authentication/example-providers/oidc/README.md

nbauernfeind · 2023-02-14T16:31:33Z

...le-providers/psk/src/main/java/io/deephaven/authentication/psk/PskAuthenticationHandler.java

+    }
+
+    @Override
+    public void initialize(String targetUrl) {


I suspect we may want to force this call to be as close to the gRPC server start() call. I'd noticed back when we were originally logging the "Anonymous access is enabled" warning that it was rather early in the start up process. Other OSS products that I've seen start web servers provide such a log message near the tail of their start-up logging. We should do the same if you haven't already addressed that.

can you and @devinrsmith discuss a bit directly here? I definitely think these two(ish) things are true:

Some authentication handlers must be logged - anon must get called out noisily, and PSK stops looking like an acceptable replacement for anon if the user has to find the url, click it, go back to the log, find the PSK, copy it, paste it into the box in the browser... Both of these are best if this is the ~last thing that is logged before we hand off control to the user.

Most other auth handlers really have no need for the target url or to log at all.

I would be in favor of having io.deephaven.server.runner.DeephavenApiServer#run be in control of the necessary logging and removing targetUrl from initialize. This gives us a chance to delay logging until after the server has started, and be smarter about when and what we actually log. This suggests we may need to use reflection (or some "hacky" means for PSK), or elevate PSK to the authentication project.

niloc132 · 2023-02-16T19:37:03Z

Possible layout for docker compose sample.

fcfb5f7

I think this is too much, even without making it clearer. The only part I want to keep (in a different commit...) is to create the /apps/libs directory when we make the images, rather than make users create it themselves - but the steps to create this are a bit too complex, unless we intend to publish the image. I think it would be better to have an explicit Dockerfile example somewhere, rather than a runnable dev docker example.

authentication/example-providers/build.gradle

devinrsmith · 2023-02-16T20:05:37Z

Do we publish these at all? that is, JAVA_PUBLIC vs JAVA_BASIC?

I see value in publishing PSK, but it's hard to see immediate value from publishing the other ones.

authType strings - okay to default to the classname for dh-provided ones, easier to user from a client (rather than guessing from the AuthHandlers value

classname sounds good

futher parameterize the oidc provider to actually be generic for any oidc provider (this implies testing with other providers), or just be explicit that this is just a 'keycloak' provider?

I'd say "no" to further parameterization at this point. May be reasonable to rename to "oidc-keycloak" or similar.

niloc132 · 2023-02-16T20:15:17Z

Out of the box, mtls, psk, and keycloak are configurable for "just limit users to those who fit this simple criteria", which solves the major feature gap we were missing here. With oidc/keycloak, an existing instance can be used by just adding a new realm or new client an an existing realm to grant/deny access, and with mtls controlling the CA used for browser or programmatic clients is at least enough to offer some guarantee over who can connect or not.

The Basic sql one is a little weaker here (though you can give it an existing postgresql database and specify the table/columns to use). It might be a stretch here, but it seems odd to skip this one, as it certainly can work out of the box (and doubles as its own tool to create passwords for the DB to store) in a simpler way than to say "either roll your own or also stand up Keycloak).

deephaven-internal · 2023-03-02T17:36:40Z

Labels indicate documentation is required. Issues for documentation have been opened:

How-to: https://github.com/deephaven/deephaven.io/issues/2243
Conceptual: https://github.com/deephaven/deephaven.io/issues/2241
Reference: https://github.com/deephaven/deephaven.io/issues/2240
Blog: https://github.com/deephaven/deephaven.io/issues/2242

niloc132 added 30 commits January 20, 2023 07:57

wip

f3c8c59

Remove unnecessary parens

ae224db

rebase damage

8cf0c68

Session expiration should be known to clients

1d9ba0e

Session expiration should be known to clients

7bd073f

Populate js login

5fa00c2

Set default in netty test, fix docs

54f6f00

Merge branch 'session-expiration-client-config' into 2564-authentication

7e3fa79

Remove unnecessary docker dependencies

356149b

Merge web-client-ide into web-client-api

445bb29

Merge branch 'simplify-js-api-build' into 2564-authentication

eeb37ac

PRototype that works

7ef31e8

Remove additional dependency

e34ab05

Merge branch 'simplify-js-api-build' into 2564-authentication

b608f80

refactor, spotless

f9f4f79

Cleanup, force console example to use psk

74292f3

missing build files for sql u/p

7663a1d

missing generated type

af14d00

Refactor so that auth constants can be requested

f578b7e

Log if a handler isn't present, at least for now

428a583

CoreClient wraps a hidden IdeConnection

874fbbf

Update samples to use login()

0d97f7b

Remove extra logging

8757908

spotless

5faa113

Merge branch 'simplify-js-api-build' into 2564-js-authentication

fab9750

Add JS API support for Flight v2 authentication

1afd1ab

Partial deephaven#2934

Working open/next bidi streaming, "correct" invoke method, and properly

82b5bd5

formed status objects

Remove unused auth constants, added refresh token event wiring

497e372

Merge remote-tracking branch 'upstream/main' into 2564-js-authentication

4405a3f

include new prop in tests

af80cb6

self-review feedback

6fee3d7

niloc132 commented Feb 7, 2023

View reviewed changes

...cket-jakarta/src/main/java/io/grpc/servlet/web/websocket/MultiplexedWebsocketStreamImpl.java Outdated Show resolved Hide resolved

This was referenced Feb 7, 2023

Java Client: Migrate off of deprecated newSession/refreshSessionToken to Flight Auth #3285

Closed

Migrates the java client authentication to Flight Auth v2 #3423

Merged

niloc132 added 2 commits February 9, 2023 09:10

Merge remote-tracking branch 'upstream/main' into 2564-authentication

1bc4760

Downgrade session logging to debug

51f0c98

devinrsmith reviewed Feb 9, 2023

View reviewed changes

niloc132 added 6 commits February 9, 2023 12:08

review feedback

03d48c5

Merge remote-tracking branch 'upstream/main' into 2564-authentication

4d539c0

Update auth samples to work with es6module api, and psk sample

cad791d

Work on packaging

06fbd41

oidc documentation

f0e98af

spotless

5f754b2

nbauernfeind reviewed Feb 14, 2023

View reviewed changes

niloc132 added 3 commits February 16, 2023 10:15

Merge branch 'main' into 2564-authentication

5f0e98f

fix javadoc

ba98d83

doc cleanup

db7def3

devinrsmith reviewed Feb 16, 2023

View reviewed changes

authentication/example-providers/build.gradle Show resolved Hide resolved

Specify version for docker-compose files

2e5a318

niloc132 marked this pull request as ready for review February 17, 2023 18:45

nbauernfeind previously approved these changes Feb 21, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into 2564-authentication

f7eee5c

niloc132 dismissed nbauernfeind’s stale review via f7eee5c March 1, 2023 18:48

nbauernfeind approved these changes Mar 2, 2023

View reviewed changes

niloc132 merged commit 8c92bff into deephaven:main Mar 2, 2023

github-actions bot locked and limited conversation to collaborators Mar 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Example Authentication providers #3410

Example Authentication providers #3410

niloc132 commented Feb 6, 2023 •

edited by nbauernfeind

Loading

devinrsmith left a comment

devinrsmith Feb 9, 2023

niloc132 Feb 9, 2023

nbauernfeind Feb 14, 2023

niloc132 Feb 16, 2023

devinrsmith Feb 9, 2023

niloc132 Feb 9, 2023

devinrsmith Feb 15, 2023

devinrsmith Feb 9, 2023

niloc132 Feb 9, 2023

devinrsmith Feb 15, 2023

niloc132 Feb 16, 2023

nbauernfeind Feb 14, 2023

nbauernfeind Feb 14, 2023

niloc132 Feb 16, 2023

devinrsmith Feb 16, 2023

niloc132 commented Feb 16, 2023

devinrsmith commented Feb 16, 2023

niloc132 commented Feb 16, 2023

deephaven-internal commented Mar 2, 2023

Example Authentication providers #3410

Example Authentication providers #3410

Conversation

niloc132 commented Feb 6, 2023 • edited by nbauernfeind Loading

devinrsmith 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

Choose a reason for hiding this comment

niloc132 commented Feb 16, 2023

devinrsmith commented Feb 16, 2023

niloc132 commented Feb 16, 2023

deephaven-internal commented Mar 2, 2023

niloc132 commented Feb 6, 2023 •

edited by nbauernfeind

Loading