security: refactor gssapi_authenticator #8416
Conversation
BenPope
force-pushed
the
kerberos-refactor-gssapi-authenticator
branch
from
255dbc9
to
2beefd1
Compare
| vlog( | ||
| seclog.warn, | ||
| "authenticate received after handshake complete {} bytes", | ||
| _state, | ||
| auth_bytes.size()); | ||
| co_return errc::invalid_gssapi_state; |
There was a problem hiding this comment.
Is there a real possibility of this happening? Would this be a candidate for a vassert?
There was a problem hiding this comment.
Is there a real possibility of this happening? Would this be a candidate for a
vassert?
The connection is open to the "public", so it avoids a type of DoS.
I tend to use vassert only if the logic is wrong, rather than bad user input
There was a problem hiding this comment.
No I understand what you mean. My question was more geared towards would the code itself be wrong if we ended up in this situation.
There was a problem hiding this comment.
More of ignorance on my part (or lack of sleep to grasp a simple situation) but I'm trying to think how an instance of authenticator get reused.
There was a problem hiding this comment.
No I understand what you mean. My question was more geared towards would the code itself be wrong if we ended up in this situation.
I guess so, but the transition to complete is pretty simple at the moment. Any transition to failed should tear down the connection anyway.
There was a problem hiding this comment.
More of ignorance on my part (or lack of sleep to grasp a simple situation) but I'm trying to think how an instance of
authenticatorget reused.
The lifetime is a subset of the connection lifetime.
| } | ||
| acl_principal get_principal_from_name(std::string_view source_name); | ||
|
|
||
| ss::sstring _primary; |
There was a problem hiding this comment.
Should this be called out specifically as the _kerberos_principal? The primary is only part of the overall principal.
There was a problem hiding this comment.
Should this be called out specifically as the _kerberos_principal? The primary is only part of the overall principal.
It's the primary of the Kerberos SPN. It's a bit close to _principal, I agree.
How about:
_krb_service_primary_rp_user_principal
There was a problem hiding this comment.
For the redpanda won't this value be "redpanda/host@REALM"? Or is it just "redpanda"?
There was a problem hiding this comment.
I'll refer you to the docs: https://github.com/BenPope/redpanda/blob/2beefd11c924456754d759c43453bdaf93eb62f9/src/v/config/configuration.cc#L899
It's a cluster config property, so it has to be the same for each broker in the cluster.
The hostname is resolved here: https://github.com/BenPope/redpanda/blob/2beefd11c924456754d759c43453bdaf93eb62f9/src/v/security/gssapi_authenticator.cc#L237-L238
There was a problem hiding this comment.
thanks sorry
No apology needed!
|
CI Failure appears to be: #8179 |
Split off the implementation details into an impl. Signed-off-by: Ben Pope <[email protected]>
Tidy up use of members: * `_rules` no longer needs passing through the chain * `_primary` and `_keytab` are now members Signed-off-by: Ben Pope <[email protected]>
Eliminate the race condition of sharing `_state` between the `gssapi_authenticator` shard and the `thread_worker` by returning the current state, and assigning it to a copy. Split the `authenticate` method between `impl` and `gssapi_authenticator`. `fail` is now called on the correct thread. Signed-off-by: Ben Pope <[email protected]>
Eliminate the race condition between `thread_worker` and the `gssapi_authenticator` shard over the shared `_principal`. Make most of the members of `impl` private. Signed-off-by: Ben Pope <[email protected]>
BenPope
force-pushed
the
kerberos-refactor-gssapi-authenticator
branch
2 times, most recently
from
235470c
to
53e1070
Compare
| _state = res.state; | ||
| co_return std::move(res.result); |
There was a problem hiding this comment.
curious why the state would change if the result had an error?
There was a problem hiding this comment.
I think most errors transition to state::failed, during the call to fail_impl., which allows failed() to work.
| "authenticate received after handshake complete {} bytes", | ||
| _state, | ||
| auth_bytes.size()); |
There was a problem hiding this comment.
mismatch in fmt arguments and fmt placeholders.
Eliminate `finish`, reset `impl` when authentication is complete. Signed-off-by: Ben Pope <[email protected]>
BenPope
force-pushed
the
kerberos-refactor-gssapi-authenticator
branch
from
53e1070
to
deaf6ff
Compare
Eliminate the race condition between the
gssapi_authenticatorshard and thethread_worker, by splitting out animplclass that only runs on theworker_thread.Fixes: #8366
Backports Required
UX Changes
Release Notes