search

guerzon / vaultwarden

Helm chart for Vaultwarden, the (unofficial) Bitwarden-compatible server written in Rust, formerly known as bitwarden_rs
MIT License
152 stars 73 forks source link

feat: Replace nginx snippet annotation with custom header annotation #106

Closed HerrSpeck closed 3 months ago

HerrSpeck commented 3 months ago

Our cluster does not allow snippet annotations by default and changing the configuration would be complicated.

But the snippet is not actually needed, since the same thing can be achieve using a config map and the custom-headers annotation.

HerrSpeck commented 3 months ago

Overall, looks good. I would just worry about breaking functionality for other users that are using Nginx Ingress with your changes.

Also, since this is a feature release, could you update the Chart.yaml to increment the minor version of the release?

Thanks :) I changed the version :) I'm not sure if it would interfere with other users. I think there was no way to really set any custom headers before. So this shouldn't override any settings? Also, the Request-Id header is still only added if the nginxIngressAnnotations are activated, which is the same behavior as before.

guerzon commented 3 months ago

Hallo @HerrSpeck, danke für die PR

I am adding some review points.

HerrSpeck commented 3 months ago

Thanks for the response! :)

You're totally right, I moved the config map to the configmap.yaml file :) and I also fixed the typo, not sure how that happened 😅

I did some digging into the global-allowed-response-headers flag and from what I can tell this is not actually a key in the custom-headers config map, but is instead part of the nginx-configuration config map. As far as I can tell, it controls which headers are allowed in general and as soon as you would like to set custom headers, you also have to specify these headers in the global-allowed-response-headers to "whitelist" them.

So I added another config map for the nginx-configuration and I put it in the same namespace as the ingress configuration. Kind of lost at this point though, because I'm not sure if that's enough for the value to be applied or if we still have to add a reference to this config map somewhere. My current understanding is that this should suffice, but maybe you can help me out if I do in fact have to reference it somewhere else :) I'm also pretty sure that setting the value of global-allowed-response-headers to just "Request-Id" would be enough, but I added the remaining default headers that you provided in the example as well for now. It might also possible to set this value directly in the values.yaml instead?

HerrSpeck commented 3 months ago

Hi there @guerzon !

I had a colleague take a look at the problem as well and we found some more information that I would like to share:

It looks like this is a flag/setting on the ingress-controller and I couldn't find any way to configure this using the project-specific charts at all. Instead, as the name suggests, this is a global configuration (like the allow-snippet-annotations flag that caused this whole issue on our side) that has to be applied directly to the ingress-controller. Therefore, I reverted the changes where I introduced an additional config map that I tried to use to allow the correct headers.

BUT: I also found out that the allow-snippet-annotations configuration is deprecated and should not be used, because of security issues. Therefore, I still think this is the correct way to solve this issue and I would suggest that we add a notice/warning to the README, that tells readers to configure the global-allowed-response-headers field of their ingress-controller to allow the Request-Id header. What do you think?

guerzon commented 3 months ago

Thanks for the feedback @HerrSpeck. I had more time to test this and I can see no issue with not adding global-allowed-response-headers.

PR looks good and ready to merge, but please rebase and fix the merge conflict. Thanks.

HerrSpeck commented 3 months ago

@guerzon Alright, sounds great :) I rebased the pr and fixed the merge conflicts. Setting 0.25.0 as the new version is alright? Or should I just increase the patch version instead?

I'm also wondering what the Request-Id header actually does? I feel like it's not actually required for the application to work. That's probably also why no issues arise if the global-allowed-response-headers field is not set.

RealKelsar commented 3 months ago

This broke my Setup. Not exactly sure why yet. Gives me 503 now, but other then in the nginx log I can not see any errors. Need to investigate after work.

guerzon commented 3 months ago

This broke my Setup. Not exactly sure why yet. Gives me 503 now, but other then in the nginx log I can not see any errors. Need to investigate after work.

Hi @RealKelsar, please submit a new issue and include your config and logs. Thanks.