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

Add support for DefaultSentinel for fallback JWT handling #6577

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

aricart
Copy link
Member

@aricart aricart commented Feb 24, 2025

Introduce a new default_sentinel option to provide a fallback JWT when none is specified in operator mode. This effectively allows for a default user, or a default callout user.

The value here must be a JWT (Bearer). If an auth-callout sentinel JWT, the user will be authenticated as an auth callout. If not, the user will be placed in the specified account.

Signed-off-by: Alberto Ricart [email protected]

@derekcollison
Copy link
Member

Have not reviewed the code, but could we just say default account, or default auth account? I assume that would make the code a bit trickier..

Also how do we feel that anyone with no creds can pound on the system here? With the bearer token it at least had a bit of resistance.

@@ -1024,6 +1025,8 @@ func (o *Options) processConfigFileLine(k string, v any, errors *[]error, warnin
*errors = append(*errors, err)
return
}
case "default_sentinel":
Copy link
Member

Choose a reason for hiding this comment

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

Probably should add checks if this is no in operator mode and if no auth callout defined, this should error?

Copy link
Member Author

@aricart aricart Feb 28, 2025

Choose a reason for hiding this comment

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

It does when it processes - same if the JWT is invalid (wouldn't be any different than the client providing the wrong JWT). Will add a validation, however this is only partial since at this point, we can know if we have an operator, but whether the user resolves or not is a different issue. The JWT can be hot-reloaded.

Copy link
Member Author

@aricart aricart Feb 28, 2025

Choose a reason for hiding this comment

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

@derekcollison added a check here
https://github.com/nats-io/nats-server/pull/6577/files#diff-4f7238cd9f97e766d6c31311be7bcc154b4d313678f513bfca50317dff080dafR72-R75

and a corresponding test - also added a test showing how it can be a default user (no callout)

@aricart
Copy link
Member Author

aricart commented Feb 28, 2025

Have not reviewed the code, but could we just say default account, or default auth account? I assume that would make the code a bit trickier..

Also how do we feel that anyone with no creds can pound on the system here? With the bearer token it at least had a bit of resistance.

This is exactly what they want to by-pass. The callout will reject as expected.

@aricart aricart force-pushed the default-sentinel branch 2 times, most recently from 0b4c4a0 to 7c6498b Compare February 28, 2025 15:39
@aricart aricart marked this pull request as ready for review February 28, 2025 16:21
@aricart aricart requested a review from a team as a code owner February 28, 2025 16:21
@neilalexander
Copy link
Member

Given the freeze, is this for 2.11.1?

@aricart
Copy link
Member Author

aricart commented Feb 28, 2025

Given the freeze, is this for 2.11.1?

Would be great if it could get in. But happy do defer.

@neilalexander
Copy link
Member

Please rebase on top of latest main and will queue up for merge, thanks!

@aricart
Copy link
Member Author

aricart commented Mar 19, 2025

@neilalexander done.

Copy link
Member

@neilalexander neilalexander left a comment

Choose a reason for hiding this comment

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

LGTM

aricart and others added 2 commits March 20, 2025 10:30
Introduce new logic to handle dynamic reloading of the `default_sentinel` configuration. Added relevant tests to validate the correct behavior of updates to this option during server reloads.

Signed-off-by: Alberto Ricart <[email protected]>
@neilalexander
Copy link
Member

=== RUN   TestConfigReloadDefaultSentinel
--- FAIL: TestConfigReloadDefaultSentinel (0.00s)
panic: No NATS Server object returned: default sentinel requires operators and accounts [recovered]
	panic: No NATS Server object returned: default sentinel requires operators and accounts
goroutine 128477 [running]:
testing.tRunner.func1.2({0x15d8f60, 0xc012837500})
	/home/travis/sdk/go1.24.1/src/testing/testing.go:1734 +0x3eb
testing.tRunner.func1()
	/home/travis/sdk/go1.24.1/src/testing/testing.go:1737 +0x696
panic({0x15d8f60?, 0xc012837500?})
	/home/travis/sdk/go1.24.1/src/runtime/panic.go:792 +0x132
github.com/nats-io/nats-server/v2/server.RunServer(0xc010bc8008)
	/home/travis/build/nats-io/nats-server/server/server_test.go:86 +0x3ce
github.com/nats-io/nats-server/v2/server.runReloadServerWithContent(0xc0129e76c0, {0xc00012e000, 0x2c, 0x200})
	/home/travis/build/nats-io/nats-server/server/reload_test.go:94 +0x93
github.com/nats-io/nats-server/v2/server.runReloadServerWithConfig(0xc0129e76c0, {0x175700f, 0x27})
	/home/travis/build/nats-io/nats-server/server/reload_test.go:86 +0x112
github.com/nats-io/nats-server/v2/server.TestConfigReloadDefaultSentinel(0xc0129e76c0)
	/home/travis/build/nats-io/nats-server/server/reload_test.go:601 +0x48
testing.tRunner(0xc0129e76c0, 0x17b6100)
	/home/travis/sdk/go1.24.1/src/testing/testing.go:1792 +0x226
created by testing.(*T).Run in goroutine 1
	/home/travis/sdk/go1.24.1/src/testing/testing.go:1851 +0x8f3
FAIL	github.com/nats-io/nats-server/v2/server	583.738s
FAIL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants