feat: Implement authc by app credential - #1187
konac-hamza wants to merge 1 commit into
Conversation
| .map_err(|_| ApplicationCredentialProviderError::AuthenticationFailed)? | ||
| .ok_or(ApplicationCredentialProviderError::AuthenticationFailed)?; | ||
|
|
||
| if !user.enabled { |
There was a problem hiding this comment.
Good that you thought about that, but it is already present in core-types::auth::UserIdentityInfo::validate to be managed centrally. We should ensure we have an appcred dedicated test though
| } | ||
|
|
||
| // --- 6. Validate the bound project is enabled --- | ||
| let project = state |
There was a problem hiding this comment.
this is also already enforced in core-types::auth/core::auth
| } | ||
|
|
||
| // --- 7. Validate the project's domain is enabled --- | ||
| let project_domain = state |
There was a problem hiding this comment.
all in all AuthenticationResult verifies that the appcred secret matches and performs rate limiting, but is not doing additional authorization validations. Those are handled by the SecurityContext
| async fn authenticate_by_application_credential<'a>( | ||
| &self, | ||
| ctx: &ExecutionContext<'a>, | ||
| id: Option<&'a str>, |
There was a problem hiding this comment.
one more auth info case is missing: user_name + domain. As such I think it would be better to define an enum which properly lists all necessary combinations: appcred_id, appcred_name + user_id, appcred_name + user_name/domain. It can be e.g.,
struct ApplicationCredentialAuthRequest {
secret: SecretString,
application_credential: ApplicationCredentialAuthData
}
enum ApplicationCredentialAuthData {
Id(ApplicationCredentialAuthById),
Name(ApplicationCredentialAuthByName)
}
struct ApplicationCredentialAuthById {
id: String,
user: Option<UserAuthRef>
}
struct ApplicationCredentialAuthByName {
name: String,
user: UserAuthRef
}
struct UserAuthRef {
pub id: Option<String>,
pub name: Option<String>,
pub domain: Option<Domain>,
}
UserAuthRefBuilder can have the validate method enforcing that id or name must be present. While the structure looks much more complex it nicely guarantees during compile time we have all possible combinations covered instead of building a chain of if-else branches. The difference to the type in the api-types is that there we define the json contract while here we want to ensure internally we have the proper information (it is actually possible model the api type with enums, but it looks very weird)
| serialize_with = "crate::common::serialize_optional_secret" | ||
| )] | ||
| #[cfg_attr(feature = "openapi", schema(value_type = Option<String>))] | ||
| pub secret: Option<SecretString>, |
There was a problem hiding this comment.
it should not be optional - secret is always mandatory and you are enforcing it in the code
There was a problem hiding this comment.
@gtema Agreed, secret is mandatory for application credential auth. The issue is that test_post_route_to_target_issues_token sends "application_credential": {"application_credential_id": "tf-alice"} without secret — serde matches the "application_credential" JSON key to the struct field and fails before the route plugin can intercept. Should I add a dummy secret to the test payload to keep secret required, or keep it as Option at the serde layer?
There was a problem hiding this comment.
@gtema I need help with the route-mode plugin compatibility. Adding the typed application_credential field to Identity breaks route plugin payload inspection — serde no longer puts it into extra, and non-standard keys like application_credential_id are lost. I'm not familiar enough with the auth plugin internals to decide the right approach here. Could you advise or pair on this part?
There was a problem hiding this comment.
thanks for noticing this. I think I have fixed this upstream so you need to rebase
There was a problem hiding this comment.
thanks for help :)
| pub struct ApplicationCredentialUser { | ||
| /// User ID. | ||
| #[cfg_attr(feature = "validate", validate(length(max = 64)))] | ||
| pub id: String, |
There was a problem hiding this comment.
as described below, but you can also integrate it here, the ApplicationCredentialUser is specified by ID and/or Name + Domain (which in turn is also specified by ID and/or Name)
| feature = "builder", | ||
| derive(derive_builder::Builder), | ||
| builder( | ||
| build_fn(error = "crate::error::BuilderError"), |
There was a problem hiding this comment.
also as mentioned below I would add here a Builder validator (https://docs.rs/derive_builder/latest/derive_builder/#pre-build-validation) to ensure id or name are present
| res.push(auth_res); | ||
| } else if method == "application_credential" { | ||
| if let Some(app_cred) = &req.auth.identity.application_credential { | ||
| let secret = app_cred |
There was a problem hiding this comment.
as mentioned above, when secret is not optional in struct this if is a no-op
| authenticate_request(state, &req, headers, peer_addr.map(|addr| addr.ip())).await?; | ||
| let ctx = SecurityContext::try_from(auth_res)?; | ||
|
|
||
| // Application credential auth: auto-derive the project scope from the |
There was a problem hiding this comment.
passing scope with appcred is prohibited by python keystone. Also when a token was retrieved using appcred further auth by token must also prohibit setting scope. From my initial intentions this must be already handled in core::auth with different variations, so that you should not bother with the logic here. But please ensure there are dedicated integration tests for this logic
3bb92cb to
9a842a7
Compare
65e02a1 to
7a4ceb7
Compare
…ntal#1242) Reference-plugin route() and its test/doc callers looked up a literal `application_credential_id` key inside the payload block. Real clients send `{"id": ..., "secret": ...}` per the actual ApplicationCredentialAuth wire shape (PR openstack-experimental#1187) - the sibling-key name only ever existed in this repo's own ADR prose and test fixtures, never in real traffic, so route-mode plugins configured to inspect application_credential would silently fall through to Passthrough for every real request. Renamed the lookup and every test/doc payload built to match it (reference-plugin fixture, ADR 0025, contributor auth-plugins doc, common.rs/create.rs test helpers, auth_plugin_auth.rs, tests/api auth_plugin.rs) to key on `id` instead. Signed-off-by: Artem Goncharov <artem.goncharov@gmail.com>
Signed-off-by: Hamza Konac <hamza.konac@tubitak.gov.tr>
7a4ceb7 to
42c67d8
Compare
Summary
Implements authentication by application credential as specified in the
[OpenStack Identity API v3](https://docs.openstack.org/api-ref/identity/v3/index.html#authenticating-with-an-application-credential).
API types: Added
ApplicationCredentialAuthandApplicationCredentialUserstructs to the auth token request types, supporting authentication by credential
ID or by name + user ID.
Provider: Added
authenticate_by_application_credentialto theApplicationCredentialApitrait andApplicationCredentialService. The methodresolves the credential, applies per-user rate limiting (ADR-0022), verifies the
secret against the stored hash via the backend driver, checks credential
expiration, and validates that the owning user, bound project, and project domain
are all enabled. Added
verify_application_credential_secretto the backendtrait with a SQL implementation.
HTTP handler: Extended
authenticate_requestto dispatch the"application_credential"method. Extendedcreate_innerto auto-derive theproject scope from the credential's
project_idwhen no explicit scope isprovided, matching the OpenStack API spec behavior.
Error mapping:
AuthenticationFailedandApplicationCredentialExpiredvariants now map to HTTP 401.
Test plan
Security review checklist
ApplicationCredential.project_idin the authentication result, not from request input.ValidatedSecurityContext::new_for_scopepath forAuthenticationContext::ApplicationCredential.ScopeInfovariant or auth method? Updatedvalidate_scope_boundaries(),calculate_effective_roles(),fully_resolved(), andCredentials::try_from? — No newScopeInfovariant.AuthenticationContext::ApplicationCredentialalready exists and is handled by all relevant match arms.This implementation was developed with AI assistance (Claude).