-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
WIP: Add authorization header support for Gitea webhooks #20267
WIP: Add authorization header support for Gitea webhooks #20267
Conversation
Signed-off-by: justusbunsi <61625851+justusbunsi@users.noreply.github.com>
Signed-off-by: justusbunsi <61625851+justusbunsi@users.noreply.github.com>
Signed-off-by: justusbunsi <61625851+justusbunsi@users.noreply.github.com>
Signed-off-by: justusbunsi <61625851+justusbunsi@users.noreply.github.com>
In case the whole auth header feature is disabled for a configured webhook, there is no need to encrypt/decrypt values. Additionally, obsolete values due to auth type switching get cleared. Signed-off-by: justusbunsi <61625851+justusbunsi@users.noreply.github.com>
- Password type - Hint regarding "token" prepend Signed-off-by: justusbunsi <61625851+justusbunsi@users.noreply.github.com>
Signed-off-by: justusbunsi <61625851+justusbunsi@users.noreply.github.com>
Signed-off-by: justusbunsi <61625851+justusbunsi@users.noreply.github.com>
Signed-off-by: justusbunsi <61625851+justusbunsi@users.noreply.github.com>
Signed-off-by: justusbunsi <61625851+justusbunsi@users.noreply.github.com>
Signed-off-by: justusbunsi <61625851+justusbunsi@users.noreply.github.com>
Signed-off-by: justusbunsi <61625851+justusbunsi@users.noreply.github.com>
Signed-off-by: justusbunsi <61625851+justusbunsi@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some things I'd like to discuss:
settings.webhook.auth_header.section = Authorization Header | ||
settings.webhook.auth_header.description = Add authorization header to webhook delivery. | ||
settings.webhook.auth_header.name = Header name | ||
settings.webhook.auth_header.type = Header content type | ||
settings.webhook.auth_header.type_basic = Basic authentication | ||
settings.webhook.auth_header.type_token = Token authentication | ||
settings.webhook.auth_header.username = Username | ||
settings.webhook.auth_header.password = Password | ||
settings.webhook.auth_header.token = Token | ||
settings.webhook.auth_header.token_description = During webhook delivery, the given value will be prepended with <code>token</code> followed by a space. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
case webhook_model.BASICAUTH: | ||
content = fmt.Sprintf("Basic %s", base.BasicAuthEncode(meta.AuthHeader.Username, meta.AuthHeader.Password)) | ||
case webhook_model.TOKENAUTH: | ||
content = fmt.Sprintf("token %s", meta.AuthHeader.Token) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be up to the admin if and what is prepended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be the use-case for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes the token must be provided with token <value>
or Bearer <value>
. Right now the second one is not possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of handling this in the backend, maybe it could be exclusively handled in the frontend:
- if the user chooses "basic auth", the frontend computes the base64 and sends
Basic <computedbase64>
- if the user chooses "bearer token", the frontend sends
Bearer <token>
- and maybe a 3rd option for the use to exactly choose the header value
This way the backend logic can be simplified a bit, while keeping maximum flexibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I also thought about frontend code for it. But IIRC, it is preferred to do most logical code in the backend. So I went that way. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That token
part definitely needs to be configurable, it's called the "schema" IIRC. Many APIs take Bearer
but in the future, other schema could be introduced.
Signed-off-by: justusbunsi <61625851+justusbunsi@users.noreply.github.com>
This could be useful for other webhooks (matrix for instance). Would it make sense to add a new dedicated |
We had a conversation in Discord about this feature a few days ago before I started implementing it and came to the conclusion that it would only be necessary for the generic webhook as all the others already have their authentication data configured. What would make the other webhooks require such headers? Maybe we missed use cases. Do you have an example? |
As far as I understand, the repo is not adding custom webhooks until #19307 is somehow resolved.
I would like to add a webhook to builds.sr.ht, which needs a Moreover this PR encrypts the token (which I think is a good idea), whereas the current implementation (in matrix for instance) stores it in plaintext. |
stupid question: is there any system that uses a different (allowing custom value for this could lead to security issues, since the request are made locally - |
Did a quick Google search for that address. Wouldn't that be a candidate for using the general webhook (Gitea webhook)?
😃 Agreed, that's why it's encrypted. If other maintainers also see it as better way to store those data, it might be changed for the other webhook types. But that would be a different PR, IMO. And definitely require a data migration. |
TBH, I don't remember any system right now, but stumbled across one in the past, IIRC. Allow to change the header name is flexible enough to support those edge cases as well but I haven't thought about changing the header name being a potential security concern. That's a fair point. Maybe such edge case is not worth changing the name. Before I hardcoded it, I'd like to have opinions from the other maintainers. |
Sorry, I mistype the URL: https://builds.sr.ht/
I don't know how much effort a data migration represents. But yes, you are right, it would be more like:
Sure! (anyway, thank you very much for taking the time to reply to my comments. I want to contribute to gitea, but I am not yet experienced with enough its codebase and its development process) |
🤔 such a webhook request require the custom webhooks, I understand what you mean. So it is not possible as of now to trigger such request from any configurable webhook type. Am I right? |
Yes, you are right. That's why I am looking at this PR and #19307, in the hope that it will enable my usecase at some point :-) |
Apparently TeamCity also needs an Authorization header: #18667 . So with matrix, sourcehut and gitea-webhook, it means at least 4 actual usecase with Regarding matrix, apparently the header is not sent on hook retry: #19872. |
From my side, there are 3 open questions:
It would be very valuable to have input from the other maintainers. I posted a message on #gitea-dev:matrix.org |
Why not make it a generic key/value map of headers that can be added to any webhook? I think it'd be more valuable and we don't need to care on the intrinsics of each header. It would mean basic auth needs to be entered in raw base64 form, but I consider that acceptable. |
Then we have to filter out or block all headers Gitea will add on its one to prevent forged headers, right? |
Yes, gitea-set headers should not be modifyable, e.g. do the go equivalent of a shallow JS merge ( Edit: |
Do you have any actual usecase for this? I fear that it would add a lot of complexity for no real usage (with possibilities of security issues, as mentioned in #20267 (comment)). Users will likely want to customize such headers depending on the hook (and not only static values). Maybe this could be linked to #19307 |
Can't think of a use case besides authorization where one want arbitrary static HTTP headers, so I'm happy if you want to keep the PR to just Authorization right now. |
<h4>{{.locale.Tr "repo.settings.webhook.auth_header.section"}}</h4> | ||
<div class="field"> | ||
<div class="ui checkbox"> | ||
<input class="hidden" name="auth_header_active" type="checkbox" tabindex="0" {{if .GiteaHook.AuthHeaderEnabled}}checked{{end}}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this tabindex
have any purpose? There are input elements before this. I'd recommend not setting it and just let the natural browser-derived tabindex take over.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😀 No, I assume that's just a copy/paste left-over.
|
||
if ($authHeaderSection.length === 0) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ($authHeaderSection.length === 0) { | |
return; | |
} | |
if ($authHeaderSection.length === 0) return; |
|
||
if (isBasicAuth) { | ||
$basicAuthFields.addClass('required'); | ||
$basicAuthFields.find('input').attr('required', ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think $basicAuthFields.find('input').prop('required', true)
would be the canonical way to set boolean attributes with jQuery as per this.
$headerName.parent().addClass('required'); | ||
$headerName.parent().show(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$headerName.parent().addClass('required'); | |
$headerName.parent().show(); | |
$headerName.parent().addClass('required').show(); |
This and similar cases can be chained for performance and conciseness.
To sum up:
I don't see an actual use case right now for complete flexibility, so I would like to keep it as authorization headers for now. All currently supported webhook don't need auth headers, so it would be this webhook type only. Custom webhooks (open PR) are a different use case where authorization headers are important. Any other improvements can be done in another PR. Ok for everyone? |
Sounds good!
The (currently supported) matrix hook has some (ugly) workaround for authorization header support. I would really like the authorization header support to be implemented for all hooks, since we identified at least 2 other systems which need them (Teamcity, Sourcehut) and probably more will come when the “custom webhook” PR is sorted out. Maybe adding an encrypted database column “authorization_header” would be a clean implementation.
I’m not sure I understand. What will be configurable, by who?
(as mentioned, I don’t see any use case for header name customization, only security issues) |
Not sure that it is an actual topic for this PR. Maybe I missed something. I'll have another, closer look at that when modifying this PR.
It's going to be a per-webhook configration. The person configuring the webhook also decides the content. Nothing internally prefixed or suffixed.
I see your point on this. No customizable header key might be safer. I need some time to continue with it. Don't have much time currently. |
I had some time lately, so I took the liberty of implementing another approach: #20926. |
_This is a different approach to #20267, I took the liberty of adapting some parts, see below_ ## Context In some cases, a weebhook endpoint requires some kind of authentication. The usual way is by sending a static `Authorization` header, with a given token. For instance: - Matrix expects a `Bearer <token>` (already implemented, by storing the header cleartext in the metadata - which is buggy on retry #19872) - TeamCity #18667 - Gitea instances #20267 - SourceHut https://man.sr.ht/graphql.md#authentication-strategies (this is my actual personal need :) ## Proposed solution Add a dedicated encrypt column to the webhook table (instead of storing it as meta as proposed in #20267), so that it gets available for all present and future hook types (especially the custom ones #19307). This would also solve the buggy matrix retry #19872. As a first step, I would recommend focusing on the backend logic and improve the frontend at a later stage. For now the UI is a simple `Authorization` field (which could be later customized with `Bearer` and `Basic` switches): ![2022-08-23-142911](https://user-images.githubusercontent.com/3864879/186162483-5b721504-eef5-4932-812e-eb96a68494cc.png) The header name is hard-coded, since I couldn't fine any usecase justifying otherwise. ## Questions - What do you think of this approach? @justusbunsi @Gusted @silverwind - ~~How are the migrations generated? Do I have to manually create a new file, or is there a command for that?~~ - ~~I started adding it to the API: should I complete it or should I drop it? (I don't know how much the API is actually used)~~ ## Done as well: - add a migration for the existing matrix webhooks and remove the `Authorization` logic there _Closes #19872_ Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com> Co-authored-by: Gusted <williamzijl7@hotmail.com> Co-authored-by: delvh <dev.lh@web.de>
Thank you for your time! @justusbunsi |
Huge thanks to @oliverpool for picking up this topic and get it over the finish line. 🥇 |
In some cases, a webhook endpoint requires some kind of authentication. In many cases it is enough to add an
Authorization
header to the webhook delivery. That's what this PR does: It allows to add such header to Gitea-typed webhooks.Feature description
The header can be defined on a per-webhook basis and is disabled by default for backwards compatibility with already configured webhooks. For specific cases the header key is adjustable but defaults to
Authorization
. There are two supported authentication types/schemes: Basic authentication and Token based authentication.Technical details
Authentication information are stored as encrypted string in the database inside the
Meta
field. If the administrator switches between the schemes and saves the page, the old settings are cleared. Temporarily switching in the UI won't clear data.I also ensured that this particular header won't be stored in the protocol for sent webhooks.
Screenshots