Authorization Callouts #3719

derekcollison · 2022-12-15T21:09:15Z

An implementation for a server-signed authorization request mechanism. The mechanism will be used in both server configuration and operator mode servers. This allows external NATS applications to securely authorize users with external AuthN.

Signed-off-by: Derek Collison derek@nats.io

/cc @nats-io/core

server/auth_callout.go

philpennock

Reviewed everything except server/auth_callout_test.go

server/accounts.go

philpennock · 2022-12-15T22:57:51Z

server/auth.go

+	// Check for server configured auth callouts.
+	if opts.AuthCallout != nil {
+		// Make sure we have a valid account and auth_users.
+		_, err := s.lookupAccount(opts.AuthCallout.Account)


Is it guaranteed that lookupAccount will be fully populated, in all account resolver modes, so that this must succeed during initial parse of the config file?

lookupAccount can reach out. Like SYSTEM account, if you are running in operator mode, this account should also be part of the preloads. But if it isn't we will try to fetch it. If that fails we will error.

server/auth.go

philpennock · 2022-12-15T23:05:21Z

server/auth.go

@@ -557,6 +578,54 @@ func (s *Server) processClientOrLeafAuthentication(c *client, opts *Options) boo
 		err  error
 		ao   bool // auth override
 	)
+
+	// Check if we have auth callouts enabled at the server level or in the bound account.
+	defer func() {


This is a lot of security-critical work to be happening inside a deferred function. I'm worried about this being triggered after a panic. Is this something which could be put into a new intermediary function instead?

This needs to be a defer to be able to reference the result of authorized which is a return variable.

server/auth_callout.go

philpennock · 2022-12-15T23:33:08Z

server/auth_callout.go

+		return
+	}
+
+	// Do it this way to fail to compile if fields are added to jwt.ClientInformation.


I don't see what would cause a compilation failure here: the other fields would just be left on their default values.

The assignment part. Left side, not right side.

server/auth_callout.go

ripienaar · 2022-12-16T16:19:05Z

I have a few comments / wishes to add here:

Can we extend the client connect event to include information that this was a user authorized by a callout. Maybe include the issuer pub key.
Can we have events about callout fails which might include reasons from the auth callout side.
Server should log that this is setup in static config mode, just a warning at start saying a callout is in use.
In this pr the nil reply is all thats accepted, I think we should consider also accepting a json document with optional error details that could be logged or included in an event (first point) to asist with debugging. JSON because we might want to extend later.
Please include server tags in server_id so I can for example say some user is only allowed to connect in the eu
More logging

derekcollison · 2022-12-16T21:24:07Z

I have a few comments / wishes to add here:

Can we extend the client connect event to include information that this was a user authorized by a callout. Maybe include the issuer pub key.

This is visible in the account of the user and not the service, so might be considered leaking info.

Can we have events about callout fails which might include reasons from the auth callout side.

Phil asked about this as well. I think starting with server logs is right approach, and we can add in events. Not opposed.

Server should log that this is setup in static config mode, just a warning at start saying a callout is in use.

Can you give me an example of what would be helpful?

In this pr the nil reply is all thats accepted, I think we should consider also accepting a json document with optional error details that could be logged or included in an event (first point) to assist with debugging. JSON because we might want to extend later.

Possibly, we would need to go back into the jwt lib and add a response claim that would include the user claim such that we could trust the error response etc. So non-trivial.

Please include server tags in server_id so I can for example say some user is only allowed to connect in the eu

Good idea. I can add that, will also require update to jwt lib.

More logging

Examples?

ripienaar · 2022-12-16T21:29:56Z

I have a few comments / wishes to add here:

Can we extend the client connect event to include information that this was a user authorized by a callout. Maybe include the issuer pub key.

This is visible in the account of the user and not the service, so might be considered leaking info.

I mean the ones that get sent to $SYS.ACCOUNT.*.CONNECT those are system only. For failures, assuming we extend the reply with error, that would go in $SYS.SERVER.*.CLIENT.AUTH.ERR

Can we have events about callout fails which might include reasons from the auth callout side.

Phil asked about this as well. I think starting with server logs is right approach, and we can add in events. Not opposed.

fair

Server should log that this is setup in static config mode, just a warning at start saying a callout is in use.

Can you give me an example of what would be helpful?

If a server config significantly change the authentication parameters of behavior that's definitely something you want to communicate and warn about. It stands out so there's no inadvertant mistakes or no surprises.

In this pr the nil reply is all thats accepted, I think we should consider also accepting a json document with optional error details that could be logged or included in an event (first point) to assist with debugging. JSON because we might want to extend later.

Possibly, we would need to go back into the jwt lib and add a response claim that would include the user claim such that we could trust the error response etc. So non-trivial.

Please include server tags in server_id so I can for example say some user is only allowed to connect in the eu

Good idea. I can add that, will also require update to jwt lib.

cool

More logging

Examples?

At debug it would for sure be good to know you're calling out and when you got a response/or not/or timeout etc. It can be hard to debug this as its a bit of effect at a distance so debug info would help developers working on auth plugins.

derekcollison · 2022-12-17T16:09:24Z

Should also add in cluster to server portion if we are adding tags.

@ripienaar for the AuthorizationResponse, should it be code + description similar to how we evolved in JetStream?

ripienaar · 2022-12-17T16:14:34Z

Don’t think it needs code - since this is not for internal use it’s user supplied code so would be hard to standardise on coded errors.

Yeah cluster would be good for sure

derekcollison · 2022-12-17T16:15:28Z

So just error as a string an empty user_claim sounds good to you?

ripienaar · 2022-12-17T16:21:31Z

So just error as a string an empty user_claim sounds good to you?

I thought maybe {"error":"X"} as a claim for now, we can also add additional fields later that way, where if we just do error string now it becomes a future pain to support both methods

server/auth.go

server/auth_callout.go

server/opts.go

derekcollison · 2022-12-27T17:11:17Z

I force pushed update that has the AuthorizationResponse now.

Still working on the correct approach to auth error events offline with the team.

This adds the ability to augment or override the NATS auth system. A server will send a signed request to $SYS.REQ.USER.AUTH on the specified account. The request will contain client information, all client options sent to the server, and optionally TLS information and client certificates. The external auth service will respond with an empty message if not authorized, or a signed User JWT that the user will bind to. The response can change the account the client will be bound to. Signed-off-by: Derek Collison <derek@nats.io>

…on from the callout service. Signed-off-by: Derek Collison <derek@nats.io>

ripienaar

LGTM, didnt have time to end to end test the new event but looks good

aricart

LGTM

Believe these were handled.

derekcollison requested review from wallyqs, ripienaar, codegangsta, bruth, philpennock, aricart, tbeets, Jarema, ColinSullivan1 and piotrpio December 15, 2022 21:09

ColinSullivan1 reviewed Dec 15, 2022

View reviewed changes

server/auth_callout.go Show resolved Hide resolved

philpennock previously requested changes Dec 15, 2022

View reviewed changes

derekcollison force-pushed the auth_callout branch from f38d8b9 to e14ce48 Compare December 16, 2022 21:17

derekcollison requested review from philpennock and ColinSullivan1 December 17, 2022 16:08

neilalexander reviewed Dec 19, 2022

View reviewed changes

server/auth.go Show resolved Hide resolved

server/auth_callout.go Show resolved Hide resolved

server/opts.go Show resolved Hide resolved

derekcollison force-pushed the auth_callout branch from e14ce48 to 8a56588 Compare December 19, 2022 17:01

derekcollison force-pushed the auth_callout branch from 8a56588 to f176c42 Compare December 27, 2022 17:09

derekcollison force-pushed the auth_callout branch 2 times, most recently from ec025ad to 3b08d96 Compare December 28, 2022 18:03

derekcollison force-pushed the auth_callout branch from 3b08d96 to 2daf904 Compare December 28, 2022 18:32

Add in account scoped auth error event. If external auth, supply reas…

a63929c

…on from the callout service. Signed-off-by: Derek Collison <derek@nats.io>

ripienaar approved these changes Jan 3, 2023

View reviewed changes

aricart approved these changes Jan 3, 2023

View reviewed changes

derekcollison merged commit 3c88947 into dev Jan 3, 2023

derekcollison deleted the auth_callout branch January 3, 2023 23:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Authorization Callouts #3719

Authorization Callouts #3719

derekcollison commented Dec 15, 2022

philpennock left a comment

philpennock Dec 15, 2022

derekcollison Dec 16, 2022

philpennock Dec 15, 2022

derekcollison Dec 16, 2022

philpennock Dec 15, 2022

derekcollison Dec 16, 2022

ripienaar commented Dec 16, 2022 •

edited

derekcollison commented Dec 16, 2022

ripienaar commented Dec 16, 2022 •

edited

derekcollison commented Dec 17, 2022 •

edited

ripienaar commented Dec 17, 2022

derekcollison commented Dec 17, 2022

ripienaar commented Dec 17, 2022

derekcollison commented Dec 27, 2022

ripienaar left a comment

aricart left a comment

Authorization Callouts #3719

Authorization Callouts #3719

Conversation

derekcollison commented Dec 15, 2022

philpennock left a comment

Choose a reason for hiding this comment

philpennock Dec 15, 2022

Choose a reason for hiding this comment

derekcollison Dec 16, 2022

Choose a reason for hiding this comment

philpennock Dec 15, 2022

Choose a reason for hiding this comment

derekcollison Dec 16, 2022

Choose a reason for hiding this comment

philpennock Dec 15, 2022

Choose a reason for hiding this comment

derekcollison Dec 16, 2022

Choose a reason for hiding this comment

ripienaar commented Dec 16, 2022 • edited

derekcollison commented Dec 16, 2022

ripienaar commented Dec 16, 2022 • edited

derekcollison commented Dec 17, 2022 • edited

ripienaar commented Dec 17, 2022

derekcollison commented Dec 17, 2022

ripienaar commented Dec 17, 2022

derekcollison commented Dec 27, 2022

ripienaar left a comment

Choose a reason for hiding this comment

aricart left a comment

Choose a reason for hiding this comment

ripienaar commented Dec 16, 2022 •

edited

ripienaar commented Dec 16, 2022 •

edited

derekcollison commented Dec 17, 2022 •

edited