-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
also use nextcloud certificate bundle when downloading from s3 #32963
Conversation
/backport to stable24 |
/backport to stable23 |
/backport to stable22 |
Hey 👋, sorry to interrupt. I've sent a pull request to revert #31574. Why? #31574 and #32963 makes it impossible to use a self signed certificate for primary object store. By default the aws sdk validate the certificates against the CA bundle provided by the operating system. I think we should keep this approach (not only for object store but every service that's essential to Nextcloud like database, redis and object store) and use the operating system's CA bundle. I read the ticket leading to this change and it looks more like a configuration issue to me than something that's wrong in our code. If someone want's to use self signed certificates - fine by me but do it the proper way: Import it into the operating system's CA bundle. Please reconsider this change 🙏 |
Thanks for the headsup, I guess a simple fix for primary storage would be to change the fallback from the "default nextcloud bundle" to the system bundle (by not overwriting it) for primary storage. As for whether or not the nextcloud bundle should be used for S3 in the first place, I'm in favor of having as many things as possible use the nextcloud bundle as it's generally easier to manage (and document since it's not distro dependent) |
Damn! I could not convince @icewind1991 😄 Let me try one more time 😎
Sounds fair. It's indeed additional work when Nextcloud relies too much on the distribution. This needs runtime checks if the given feature is available, the right version of a system library is installed and such cases are in general hard to debug. A recent example is #28105 which turn's out a problem (for most) in PHP. And still is my favorite solution a good integration with the operating system here. Why? Let's pretend I'm a system administrator. I have to manage our internal network and we decided to set up our own CA to issue certificates for our internal services. This sounds a bit old school since Let's Encrypt & Co. are available but it still makes sense for some people. We deploy the CA certificate to all our clients and servers. This will work out of the box for many applications. Postfix, Dovecot, Apache2, Nginx, MariaDB, etc. everyone is using the CA bundle provided by the operating system to validate the certificates. And that is what I as a system administrator expect. If necessary I can reconfigure every service to use a different bundle or disable validation at all but by default, it uses the provided bundle. In addition, it's easier to spot misconfiguration as every service is using the same data. Do you want to check if the connection to a service with your self-signed certificate works? Just use curl (which is also using the default bundle). It's not obvious that Nextcloud might use its own bundle.
Let's assume the same customer want's to mount an internal sftp service. It will not work due to a certificate error? But it works for the primary storage. They need to import their internal CA via our certificate manager. But that's not required for the primary storage. I find that confusing 🙈 I don't want to use too much of our time for this debate. We both have good points to do the one or the other approach. Please let me know how you or the server team decides. |
I guess the best option is to add a config flag to the external storage config to enable using the nc certificate bundle |
Signed-off-by: Robin Appelman <[email protected]>
Signed-off-by: Robin Appelman <[email protected]>
99f62e0
to
f148689
Compare
LGTM 🎉 Thanks for taking care @icewind1991 👍 |
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 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 I hit this issue or a related one with #50098 . I understand this merge introduces a flag |
S3 reads don't go trough the regular s3 http client for streaming reasons so it was missed by the changes in #31574