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

security/oidc: Basic support for OIDC with Kafka and HTTP #14378

Merged
merged 15 commits into from
Oct 31, 2023

Conversation

BenPope
Copy link
Member

@BenPope BenPope commented Oct 23, 2023

Add basic support for:

  • Kafka API: SASL/OAUTHBEARER
  • HTTP APIs (SR/PP/Admin): OpenID Connect

The principal mapped from the sub claim of the JWT.

Docs

New cluster configuration options:

option description default
oidc_discovery_url "The URL pointing to the well-known discovery endpoint for the OIDC provider." https://auth.prd.cloud.redpanda.com/.well-known/openid-configuration
oidc_token_audience "A string representing the intended recipient of the token." redpanda
oidc_clock_skew_tolerance "The amount of seconds to allow for when validating the exp, nbf, and iat claims in the token." 30s
http_authentication "A list of supported HTTP authentication mechanisms. BASIC and OIDC are allowed." ["BASIC"]
sasl_mechanisms "A list of supported SASL mechanisms. SCRAM, GSSAPI, and OAUTHBEARER are allowed." ["SCRAM"]

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v23.2.x
  • v23.1.x
  • v22.3.x

Release Notes

Features

  • Kafka API: Support SASL/OAUTHBEARER
  • HTTP Proxy: Support OpenID connect
  • Schema Registry: Support OpenID connect
  • Admin API: Support OpenID connect

@BenPope BenPope added area/kafka area/docs area/pandaproxy REST interface for Kafka API area/schema-registry Schema Registry service within Redpanda area/security area/admin-api labels Oct 23, 2023
@BenPope BenPope self-assigned this Oct 23, 2023
@BenPope BenPope marked this pull request as ready for review October 23, 2023 21:01
Copy link
Member

@oleiman oleiman left a comment

Choose a reason for hiding this comment

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

Probably benefit from a second set of eyes, but lgtm

src/v/security/oidc_authenticator.h Show resolved Hide resolved
Comment on lines 345 to 352
// Incorrect issuer
{1695887942,
R"({"iss": "wrong", "sub": "subject", "aud": "redpanda", "exp": 1695887942, "iat": 1695887942})",
"http://docker-rp-1:8080/realms/demorealm",
"redpanda",
0s,
oidc::errc::jwt_invalid,
security::acl_principal{security::principal_type::user, "subject"}},
Copy link
Member

Choose a reason for hiding this comment

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

Case repeated from above?

Copy link
Member Author

Choose a reason for hiding this comment

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

Case repeated from above?

No?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm with @oleiman

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm with @oleiman

Me too, GitHub UI was being silly

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

src/v/security/oidc_service.cc Outdated Show resolved Hide resolved

co_return co_await http::with_client(
std::move(client),
[req_hdr](auto& client) mutable -> ss::future<ss::sstring> {
Copy link
Member

Choose a reason for hiding this comment

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

[req_hdr = std::move(req_hdr)]?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

"invalid response from discovery_url: {}, errc: {}",
response_body,
metadata.error());
co_return; // errc::invalid_credentials;
Copy link
Member

Choose a reason for hiding this comment

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

Is the commented code intended as inline documentation or cruft?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is the commented code intended as inline documentation or cruft?

Looks old,

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed

tests/rptest/tests/redpanda_oauth_test.py Show resolved Hide resolved
"Unauthorized", ss::http::reply::status_type::unauthorized);
}
const auto& superusers = _superusers();
auto found = std::find(superusers.begin(), superusers.end(), username);
Copy link
Member

Choose a reason for hiding this comment

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

should username be initialized somewhere on the oidc path?

Copy link
Member Author

Choose a reason for hiding this comment

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

should username be initialized somewhere on the oidc path?

Good spot. Looks like I've dropped some tests in this area.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is all different, and fixed.

tests/rptest/tests/redpanda_oauth_test.py Outdated Show resolved Hide resolved
Copy link
Contributor

@michael-redpanda michael-redpanda left a comment

Choose a reason for hiding this comment

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

looking nice so far!

src/v/security/oidc_authenticator.cc Outdated Show resolved Hide resolved
src/v/security/oidc_authenticator.cc Outdated Show resolved Hide resolved
src/v/security/oidc_service.cc Outdated Show resolved Hide resolved
src/v/security/oidc_service.cc Outdated Show resolved Hide resolved
src/v/security/oidc_service.cc Outdated Show resolved Hide resolved
src/v/security/oidc_service.cc Show resolved Hide resolved
tests/rptest/tests/redpanda_oauth_test.py Outdated Show resolved Hide resolved
src/v/security/oidc_authenticator.cc Outdated Show resolved Hide resolved
src/v/security/oidc_authenticator.cc Outdated Show resolved Hide resolved
src/v/config/configuration.cc Show resolved Hide resolved
@BenPope
Copy link
Member Author

BenPope commented Oct 25, 2023

Changes in force-push

  • Improve use of result
    • Use assume_error when has_error was checked.
  • Improve oidc::errc
    • enum class
    • Finer-grained errors
  • Improve logging
    • Remove logging that has no context by leveraging new errc
    • Increase log-level for failed auth
    • Use std::error_code::message
  • Improve validation for oidc_discovery_url
  • Improved error handling for oidc_service::update
  • kafka/client now supports SASL/OAUTHBEARER for principal propagaton with HTTP REST
  • SR and PP tests

@BenPope BenPope force-pushed the oidc_authenticator branch 2 times, most recently from b603029 to 7348305 Compare October 25, 2023 22:45
@BenPope
Copy link
Member Author

BenPope commented Oct 25, 2023

Changes in force-push

  • Rebase

@BenPope
Copy link
Member Author

BenPope commented Oct 25, 2023

Changes in force-push

  • Improved logging for sasl_authenticator

@BenPope
Copy link
Member Author

BenPope commented Oct 25, 2023

Changes in force-push

  • Fix lint error due to rebase

@vbotbuildovich
Copy link
Collaborator

new failures detected in https://buildkite.com/redpanda/redpanda/builds/39826#018b6935-4362-467c-9095-387837653e59: "rptest.tests.cluster_config_test.ClusterConfigTest.test_valid_settings"

@vbotbuildovich
Copy link
Collaborator

new failures detected in https://buildkite.com/redpanda/redpanda/builds/39826#018b6943-5fab-42b2-881a-1b6466ffeb6e: "rptest.tests.cluster_config_test.ClusterConfigTest.test_valid_settings"

@vbotbuildovich
Copy link
Collaborator

@piyushredpanda
Copy link
Contributor

/ci-repeat 1

@BenPope BenPope requested a review from a team as a code owner October 30, 2023 19:34
@BenPope BenPope requested review from andrewhsu and removed request for a team October 30, 2023 19:34
@BenPope
Copy link
Member Author

BenPope commented Oct 30, 2023

Changes in force-push

  • Rebase (sorry - wasm changes forced it)
  • Add ada-url to cmake/dependencies.cmake
  • Introduce security/oidc_url_parser and tests
  • config now relies on oidc_url_parser instead of Boost.URL
  • security::oidc:make_reques now relies on oidc_url_parser

@dotnwat
Copy link
Member

dotnwat commented Oct 30, 2023

Rebase (sorry - wasm changes forced it)

a hack that i've seen Tyler use recently is to push the non-rebased version with the conflict, then do something like leave a comment which helps convince github to not squash the change history, and then push the rebased version. then the link to the minimal diff is preserved.

@dotnwat
Copy link
Member

dotnwat commented Oct 30, 2023

build-oss seems to still have some issues

Signed-off-by: Ben Pope <ben@redpanda.com>
Signed-off-by: Ben Pope <ben@redpanda.com>
Signed-off-by: Ben Pope <ben@redpanda.com>
Signed-off-by: Ben Pope <ben@redpanda.com>
Signed-off-by: Ben Pope <ben@redpanda.com>
Signed-off-by: Ben Pope <ben@redpanda.com>
Signed-off-by: Ben Pope <ben@redpanda.com>
Signed-off-by: Ben Pope <ben@redpanda.com>
Signed-off-by: Ben Pope <ben@redpanda.com>
Signed-off-by: Ben Pope <ben@redpanda.com>
@michael-redpanda
Copy link
Contributor

build-oss seems to still have some issues

Looks like it's fixed, great stuff @BenPope

@dotnwat dotnwat dismissed their stale review October 30, 2023 22:11

build-oss happy

@vbotbuildovich
Copy link
Collaborator

Copy link
Contributor

@michael-redpanda michael-redpanda left a comment

Choose a reason for hiding this comment

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

lgtm

}
result<parsed_url> parse_url(std::string_view url_view) {
parsed_url result;
auto url = ada::parse<ada::url_aggregator>(url_view);
Copy link
Member

Choose a reason for hiding this comment

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

nice

@BenPope BenPope merged commit 0dfb673 into redpanda-data:dev Oct 31, 2023
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants