Per domain authorization for certificate issuance #3889
Conversation
charhate
requested review from
csine-pro,
douglasc-nflx,
hosseinsh and
jtschladen
as code owners
| """ | ||
| if current_app.config.get("USER_DOMAIN_AUTHORIZATION_PROVIDER") is None: | ||
| # nothing to check since USER_DOMAIN_AUTHORIZATION_PROVIDER is not configured | ||
| return |
There was a problem hiding this comment.
would we want to return True here, per above description
There was a problem hiding this comment.
Thanks for catching, modified description (additional details)
|
|
||
| if error: | ||
| raise error | ||
|
|
There was a problem hiding this comment.
we are not returning the outcome if is_authorized()?
There was a problem hiding this comment.
Regarding propagation of authz result, it instead relies on raising UnauthorizedError. Raising error with useful details within custom plugin implementation can be useful to return an actionable message when certificate issuance is denied. The exception will get caught at certificate create Post call to return 403 along with the error message.
I did some minor changes to above method
- look at
authorizedboolean, raiseUnauthorizedError()if authz returns False - update description to reflect behavior
| if isinstance(san, x509.DNSName): | ||
| if san.value == common_name: | ||
| check_permission_for_cn = False | ||
| is_authorized_for_domain(san.value) |
There was a problem hiding this comment.
not returning the outcome of the AuthZ evaluation?
There was a problem hiding this comment.
It raises UnauthorizedError if authz fails (additional details)
|
|
||
| # lemur UI copies CN as SAN (x509.DNSName). Permission check for CN might already be covered above. | ||
| if check_permission_for_cn: | ||
| is_authorized_for_domain(common_name) |
There was a problem hiding this comment.
not returning the outcome of the AuthZ evaluation?
|
|
||
| data["creator"] = g.user | ||
| try: | ||
| service.allowed_issuance_for_domain(data["common_name"], data["extensions"]) |
There was a problem hiding this comment.
same question as above about the outcome of the authZ eval
There was a problem hiding this comment.
It raises UnauthorizedError if authz fails (additional details), which gets caught below to return 403 along with error message
| 'lemur.certificates.service.is_authorized_for_domain', side_effect=mocked_is_authorized_for_domain | ||
| ) as wrapper: | ||
| try: | ||
| allowed_issuance_for_domain(common_name, extensions) |
There was a problem hiding this comment.
no return values being tested?
There was a problem hiding this comment.
asserting on exception below (additional details)
Introducing new Plugins
AuthorizationPlugin(Plugin)andDomainAuthorizationPlugin(AuthorizationPlugin). One can implement aDomainAuthorizationPluginand configure its slug asUSER_DOMAIN_AUTHORIZATION_PROVIDERto check if caller is authorized to issue a certificate for a given Common Name and Subject Alternative Name (SAN) of type DNSName.Users authenticate to Lemur either using the API Key or using one of the authentication providers. Over the time, we have seen API keys being used by applications to talk to Lemur instead of direct user interaction. It seems logical to persist this information in Lemur for tracking purpose. Also it can be used for authorization. e2d406ada25c_.py aims to add a new column
application_nametoapi_keystable. Null values are allowed making sure this change in backward compatible.