Alert - Support for authentication when calling receiver endpoint. #560
Conversation
…cation' into feat/alert-authentication
There was a problem hiding this comment.
Thanks a lot for this awesome PR! This is really useful, and also good quality.
I added a few suggestions on how to improve the code, especially structure it so that there is less repetition.
@xopham I did not review schema definitions, tests, and docs.
connaisseur/alert.py
Outdated
|
|
||
| def __init__(self, alert_receiver_config: dict): | ||
| basic_authentication_config = alert_receiver_config.get( | ||
| "receiver_authentication_basic", None |
There was a problem hiding this comment.
None is the default for dict.get. Same for other occurrences.
connaisseur/alert.py
Outdated
|
|
||
| if ( | ||
| basic_authentication_config is None | ||
| ): # TODO maybe remove this check since it is included in the json validation? |
connaisseur/alert.py
Outdated
| ) | ||
|
|
||
| self.authorization_prefix = basic_authentication_config.get( | ||
| "authorization_prefix", "Basic" |
There was a problem hiding this comment.
Should the default be self.authorization_prefix, as the value is hardcoded at the class?
connaisseur/alert.py
Outdated
| self.authorization_prefix = basic_authentication_config.get( | ||
| "authorization_prefix", "Basic" | ||
| ) | ||
| # TODO maybe validate authorization prefix |
There was a problem hiding this comment.
I cannot find an actual validation rule.
The only thing I can found is: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Authorization.
I have assumed that the authentication scheme (previously called authorization prefix) is not null or empty and does not contain any space.
connaisseur/alert.py
Outdated
| token_env is not None and token_file is not None | ||
| ): # TODO maybe remove this check since it is included in the json validation? | ||
| raise ConfigurationError( | ||
| "Both token_env and token_file configuration found. Only one is required." |
connaisseur/alert.py
Outdated
| bearer_authentication_config = alert_receiver_config.get( | ||
| "receiver_authentication_bearer", None | ||
| ) | ||
|
|
||
| if ( | ||
| bearer_authentication_config is None | ||
| ): # TODO maybe remove this check since it is included in the json validation? | ||
| raise ConfigurationError( | ||
| "No bearer authentication configuration found." | ||
| ) |
There was a problem hiding this comment.
This is boilerplate (together with assigning self.authentication_prefix at the end of the method).
Suggestion: make a base class, introduce a variable for the lookup key (e.g. "receiver_authentication_bearer"), and run all this via super().__init__(*args, **kwargs).
connaisseur/alert.py
Outdated
| if self.is_basic(): | ||
| self.__init_basic_authentication(alert_receiver_config) | ||
| elif self.is_bearer(): | ||
| self.__init_bearer_authentication(alert_receiver_config) |
There was a problem hiding this comment.
init_map = {
"basic": AlertReceiverAuthentication.AlertReceiverBasicAuthentication,
"bearer": AlertReceiverAuthentication.AlertReceiverBearerAuthentication,
}
try:
self.auth_instance = init_map[self.authentication_type](alert_receiver_config)
except AttributeError:
....Then, it get_auth_header(), you don't need to do the case handling, but can just return {} if self.auth_instance is None self.auth_instance.get_header().
| @@ -89,4 +235,4 @@ | |||
| ] | |||
| } | |||
| } | |||
| } | |||
| } | |||
|
@peterthomassen I have updated the alert classes accordingly to your suggestions. |
…cation' into feat/alert-authentication
connaisseur/alert.py
Outdated
|
|
||
| class AlertReceiverAuthenticationInterface: | ||
| def __init__(self, alert_receiver_config: dict, authentication_key: str): | ||
| self.authentication_config = alert_receiver_config.get(authentication_key) |
There was a problem hiding this comment.
authentication_key sounds like a credential
There was a problem hiding this comment.
Yes, I agree. It will be called authentication_config_key.
connaisseur/alert.py
Outdated
| "The authentication scheme cannot be null or empty." | ||
| ) | ||
|
|
||
| if " " in self.authentication_scheme: |
There was a problem hiding this comment.
You might want to check that it contains only a-zA-Z or something like that. (" " is very specific, doesn't catch tabs, newlines, ...)
connaisseur/alert.py
Outdated
| def get_header(self) -> dict: | ||
| return {} |
There was a problem hiding this comment.
This could be the default implementation (instead of @abstractmethod). (Just a thought, certainly not mandatory.)
connaisseur/alert.py
Outdated
| if self.authentication_type not in AlertReceiverAuthentication.init_map.keys(): | ||
| raise ConfigurationError( | ||
| f"No authentication type found. Valid values are {list(AlertReceiverAuthentication.init_map.keys())}" | ||
| ) # hopefully this never happens | ||
|
|
||
| def is_none(self): | ||
| return self.authentication_type == "none" | ||
|
|
||
| def __init_bearer_authentication(self, alert_receiver_config: dict): | ||
| self.bearer_authentication = ( | ||
| AlertReceiverAuthentication.AlertReceiverBearerAuthentication( | ||
| alert_receiver_config | ||
| ) | ||
| ) | ||
|
|
||
| def __init_basic_authentication(self, alert_receiver_config: dict): | ||
| self.basic_authentication = ( | ||
| AlertReceiverAuthentication.AlertReceiverBasicAuthentication( | ||
| alert_receiver_config | ||
| ) | ||
| ) | ||
| return self.init_map.get(self.authentication_type) |
There was a problem hiding this comment.
try:
return self.init_map[self.authentication_type]
except KeyError:
raise ConfigurationError(...)This is a Python paradigm called EAFP. For context, see https://sauravmaheshkar.github.io/blog/Introduction-To-Duck-Typing-and-EAFP/.
One benefit is that EAFP gets ride of many TOCTOU race conditions (although not the case here, as the state is purely local).
There was a problem hiding this comment.
I was not aware of this python principle. Thank you.
There was a problem hiding this comment.
@andreee94 real neat contribution 🚀 like the work you are contributing here 🙏
there is still a few pylint findings:
connaisseur/alert.py:102:0: C0301: Line too long (107/105) (line-too-long)
connaisseur/alert.py:127:0: C0301: Line too long (116/105) (line-too-long)
connaisseur/alert.py:205:0: C0301: Line too long (117/105) (line-too-long)
connaisseur/alert.py:94:8: W0231: __init__ method from base class 'AlertReceiverAuthenticationInterface' is not called (super-init-not-called)
connaisseur/alert.py:172:25: W1514: Using open without explicitly specifying an encoding (unspecified-encoding)
connaisseur/alert.py:175:20: W0707: Consider explicitly re-raising using the 'from' keyword (raise-missing-from)
connaisseur/alert.py:177:20: W0707: Consider explicitly re-raising using the 'from' keyword (raise-missing-from)
connaisseur/alert.py:203:43: C0201: Consider iterating the dictionary directly instead of calling .keys() (consider-iterating-dictionary)
will take a closer look at the pr once @peterthomassen is done.
connaisseur/alert.py
Outdated
| self.authentication_config = alert_receiver_config.get( | ||
| authentication_config_key | ||
| ) | ||
|
|
||
| self.authentication_scheme = self.authentication_config.get( | ||
| "authentication_scheme", self.authentication_scheme | ||
| ) | ||
| self._validate_authentication_scheme() | ||
| if self.authentication_config is None: | ||
| raise ConfigurationError( | ||
| "No authentication configuration found for dictionary key:" | ||
| f"{authentication_config_key}." | ||
| ) |
There was a problem hiding this comment.
Here, you can also try: self.authentication_config = alert_receiver_config[authentication_config_key], and then raise the error in the except: block.
There was a problem hiding this comment.
Fixed also other places where EAFP can be applied.
There was a problem hiding this comment.
Cool, thanks! I think the code looks great now. Let's hand over to @xopham who will think about this more from the functionality side.
Hope you didn't mind my nit-picking :-)
| self.authentication_scheme = self.authentication_config.get( | ||
| "authentication_scheme", self.authentication_scheme | ||
| ) |
There was a problem hiding this comment.
What's the second argument here? self.authentication_scheme would be None at this point, which is the default for dict.get.
There was a problem hiding this comment.
Actually it should take the default value set in the classes that inherited the base.
This is a very bad coded concept, but it should work:
class Base:
def __init__(self):
self.surname = self.name if self.name is not None else 'Base'
class A(Base):
name = "A"
def __init__(self):
super().__init__()
class B(Base):
name = "B"
def __init__(self):
super().__init__()
a = A()
print(a.surname)
b = B()
print(b.surname)Which prints:
A
BA proper nit-picking review is a great opportunity to improve my coding skills ;)
There was a problem hiding this comment.
@andreee94 thanks for contributing! Really neat work. I think reworking the authentication is definitely required for the alerting. When first introduced, it was intentionally left open via the custom_header field as authentication can be very divers and is hard to pin down without just accepting anything. However, as such any authorization header must be provided within the values.yaml which is not ideal for a secret.
Overall the alerting configuration is somewhat complex and may need reworking eventually.
I made a few comments, however I would like to discuss the general layout:
- there is a wealth of types of authorization headers which makes this difficult to design. However, as a fallback people can always use the
custom_headersfield. Thus, I think we are fine restricting us here toBearerorBasic - in the PR the fields
receiver_authentication_type,receiver_authentication_bearerandreceiver_authentication_basicare introduced which has redundancy in the authentication type (essentially, specifyingreceiver_authentication_beareralready defines thereceiver_authentication_typewhich is thus obsolete) - there is two ways how secrets can be specified:
- via environment variables: while better than plain in the
values.yaml, this is still not a native way of providing a secret in Kubernetes. The intentional solution would be a kubernetes secret. If we are to improve the security, we should go for the intended handling of secrets. - via the service account token: Unless there is concrete use-case (e.g. intentional usage of the account token in a managed kubernetes as a workload identity), I would caution against this, as (1) in the current form this allows accessing a random path on the pod and (2) this would exfiltrate the service account token that has a much broader permission out of the scope of the cluster in question.
- via environment variables: while better than plain in the
- the field names seem long in the current form
Providing a concise, clear and compatible form seems to be very challenging and this is further complicated by the complex configuration of alerting. I would propose using the following form:
alerting:
admit_request:
templates:
- template: ecs-1-12-0
receiver_url: https://your.custom.domain.com/webhook/admit
authorization_header:
scheme: Bearer # scheme (e.g. `Bearer`, `Basic`) used in authorization header
# either (preferred)
secret_name: alladins-secret # specify the name of a Kubernetes secret to be used. The secret take the following form: `auth: base64(opensesame)` (in case of basic auth use base64(username:password)
# or
auth: opensesame # secret to add after the scheme in the authorization header (in case of basic auth use base64(username:password)The proposal is similar to the auth for validators. It reduces redundancy, avoids having to parse username and password that are provided as a single token anyways and allows either rendering the auth field in during deployment via helm install ... -set ... or employing an actual kubernetes secret that must be configured separately. In case, the validation for scheme was chosen as any string containing only letters and '-' with at maximum 20 characters, this would also allow more exotic auth headers such as OpsGenie's "authorization: GenieKey reallybadchoicetobeuniqueinaworldofstandars`. The kubernetes secret could still be added to Connaisseur as an env variable.
I apologize for the long response, however it took me some time to wrap my head around this and I am still not certain whether this is the optimal solution. Let me know what you think 🙏
docs/features/alerting.md
Outdated
| @@ -52,6 +61,54 @@ alerting: | |||
| receiver_url: https://bots.keybase.io/webhookbot/<Your-Keybase-Hook-Token> | |||
| ``` | |||
|
|
|||
| ## Example With Authentication | |||
There was a problem hiding this comment.
I'd prefer to make this a new subsection of additional notes
| token_env: CONNAISSEUR_ADMIT_REQUEST_WEBHOOK_AUTH_TOKEN | ||
| ``` | ||
|
|
||
| You then have to set the `CONNAISSEUR_ADMIT_REQUEST_WEBHOOK_AUTH_TOKEN` environment variable referencing the bearer token secret you want to use into the connaisseur deployment. |
There was a problem hiding this comment.
to understand this correctly, one would have to provide the secret here and below via the .deployment.envs, right in helm/values.yaml or se them up manually in the kubernetes connaisseur namespace, right? If that is so, we should specifically point those options out here and below.
There was a problem hiding this comment.
Actually I am modifying the deployment.yamlfile in the helm chart to include the possibility to load envs from secrets:
{{- if .Values.deployment.env }}
{{- range $e := .Values.deployment.env }}
- name: {{ $e.name }}
value: {{ $e.value }}
{{- end }}
{{- end }}
{{- if .Values.deployment.envValueFrom}}
{{- range $e := .Values.deployment.envValueFrom }}
- name: {{ $e.name }}
valueFrom:
{{- if $e.valueFrom.secretKeyRef }}
secretKeyRef:
name: {{ $e.valueFrom.secretKeyRef.name }}
key: {{ $e.valueFrom.secretKeyRef.key }}
{{- else if $e.valueFrom.configMapKeyRef }}
configMapKeyRef:
name: {{ $e.valueFrom.configMapKeyRef.name }}
key: {{ $e.valueFrom.configMapKeyRef.key }}
{{- else if $e.valueFrom.fieldRef }}
fieldRef:
fieldPath: {{ $e.valueFrom.fieldRef.fieldPath }}
{{- end }}
{{- end }}
{{- end }}deployment:
# Add the environmental variables to the pod in the deployment
# NOT use this for secrets, use the envValueFrom section instead
env:
- name: CONNAISSEUR_ENV
value: "admin"
# Add environmental variables to the pod in the deployment
# referencing secrets, configMaps, or fields.
envValueFrom:
- name: CONNAISSEUR_WEBHOOK_USERNAME
valueFrom:
secretKeyRef:
name: connaisseur-webhook-user
key: username
- name: CONNAISSEUR_WEBHOOK_PASSWORD
valueFrom:
secretKeyRef:
name: connaisseur-webhook-user
key: passwordI will commit the code shortly.
The .deployment.envs option is fine for non secrets environmental variables.
docs/features/alerting.md
Outdated
| receiver_url: https://your.custom.domain.com/webhook/admit | ||
| receiver_authentication_type: bearer | ||
| receiver_authentication_bearer: | ||
| token_file: /var/run/secrets/kubernetes.io/serviceaccount/token |
There was a problem hiding this comment.
this is certainly an option, but I would consider that somewhat bad practice to misuse the service account token that has a much broader scope than authentication of the alerting webhook. One would technically be sending a kubernetes secret out of the cluster. What would be the exact use-case of this? Is there a situation where this token would be a reasonable secret? Is there a managed k8s provider where such an authentication is possible?
There was a problem hiding this comment.
besides, this is somewhat guessing where a file will be and will only really work for the serviceaccount token. as such it should then not be a file but a boolean whether to use the service account token or not.
Or how would one get a file with a secret into the container?
There was a problem hiding this comment.
This is just an example of loading the token from the service account. Since the service account token is just a file inside the pod I prefer not to explicitly have a bool to control this behavior but to let it generic.
For what concern the service account as authentication method, I am not sure if it makes sense to use it in a production environment with the webhook inside the cluster (To not let the token to leak outside).
I have just tried it to experiment with the TokenReview and the system:auth-delegator cluster role.
Taking the token from a file may be used in general if the secret has been mounted as file inside the pod.
To make the use case more clear I can change the example to something more generic like /etc/webhook/token.
docs/features/alerting.md
Outdated
| | `alerting.<category>.receiver_authentication_bearer` | object | - | only when `receiver_authentication_type` is `bearer` | Authentication credentials for bearer authentication. | | ||
| | `alerting.<category>.receiver_authentication_bearer.token_env` | string | - | only when `receiver_authentication_type` is `bearer` | Token Environmental variable for bearer authentication (Exclusive with `token_file`). | | ||
| | `alerting.<category>.receiver_authentication_bearer.token_file` | string | - | only when `receiver_authentication_type` is `bearer` | Token file for bearer authentication (Exclusive with `token_env`). | | ||
| | `alerting.<category>.receiver_authentication_bearer.authentication_scheme` | string (without spaces) | `Bearer` | | Prefix for Authorization header for bearer authentication. | |
There was a problem hiding this comment.
I'd propose to reference the subsection below for details. see e.g. here
| @@ -57,6 +58,76 @@ | |||
| "template": "custom", | |||
| } | |||
|
|
|||
| receiver_config_bearer_env = { | |||
There was a problem hiding this comment.
the added tests seem to not test several of the added configurationerrors above. Pytest claims the following lines are not being tested:
$ pytest tests --cov-report term-missing --cov connaisseur
(...)
Name Stmts Miss Cover Missing
-------------------------------------------------------------------------------------
(...)
connaisseur/alert.py 181 11 94% 66-67, 115-116, 151, 158, 177-178, 205, 285-286
(...)
There was a problem hiding this comment.
Most of the not tested lines are part of the code that should not be reached since the validation is performed with the schema validation:
- 66-67
- 115-116
- 151
- 158
- 205
This is in case the file reading fails for unknown reason (FileNotFoundError tested separately).
The approach mirrors the code already used to load a file in lines 277-288:
- 177-178
These are lines not concerning this pull request:
- 241-243
- 285-286
docs/features/alerting.md
Outdated
|
|
||
| Or if you would like to use the service account token as the bearer token, you can use the following snippet: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
here and in other cases should be denoted as YAML
| ``` | |
| ```yaml |
…al notes section.
… into the connaisseur pod.
…cation' into feat/alert-authentication
Codecov Report
@@ Coverage Diff @@
## develop #560 +/- ##
===========================================
+ Coverage 93.52% 96.17% +2.64%
===========================================
Files 15 22 +7
Lines 633 1254 +621
===========================================
+ Hits 592 1206 +614
- Misses 41 48 +7
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
@andreee94 sorry for taking so long with feedback. I was very busy and we had to fix a few bugs with high priority recently. I hope to get back to this next week.
The changes implement more authentication options without requiring to hard-code the secret inside the url.
Until now is not possible to call an alert webhook which requires a basic or a bearer authentication.
The secrets should not be passed directly inside the configuration, instead they can be injected as environmental variables or files.
Description
Three authentication options have been implemented:
Is however possible to specify a custom header prefix other than Basic and Bearer.
The validation schema has been updated to support the new options and the unit test has been written. All tests pass (running them inside the alpine container as suggested by the documentation).
The documentation reports a description of the new functionalities and few examples.
The new connaisseur image has been installed manually (forked from master) inside a K3S cluster and it worked correctly .
Checklist
developvalues.yamlandChart.yaml(if necessary)