Skip to content
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

Merged
merged 2 commits into from
Jul 18, 2022

Conversation

icewind1991
Copy link
Member

S3 reads don't go trough the regular s3 http client for streaming reasons so it was missed by the changes in #31574

@icewind1991 icewind1991 added the 3. to review Waiting for reviews label Jun 21, 2022
@icewind1991 icewind1991 added this to the Nextcloud 25 milestone Jun 21, 2022
@icewind1991 icewind1991 requested review from a team, PVince81, blizzz and CarlSchwan and removed request for a team June 21, 2022 14:51
@icewind1991
Copy link
Member Author

/backport to stable24

@icewind1991
Copy link
Member Author

/backport to stable23

@icewind1991
Copy link
Member Author

/backport to stable22

@kesselb
Copy link
Contributor

kesselb commented Jun 21, 2022

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 🙏

@icewind1991
Copy link
Member Author

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)

@kesselb
Copy link
Contributor

kesselb commented Jun 24, 2022

Damn! I could not convince @icewind1991 😄 Let me try one more time 😎

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)

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.

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.

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.

@icewind1991
Copy link
Member Author

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 <robin@icewind.nl>
Signed-off-by: Robin Appelman <robin@icewind.nl>
@kesselb
Copy link
Contributor

kesselb commented Jul 6, 2022

I guess the best option is to add a config flag to the external storage config to enable using the nc certificate bundle

LGTM 🎉 Thanks for taking care @icewind1991 👍

Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐘

Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@PVince81 PVince81 added 4. to release Ready to be released and/or waiting for tests to finish regression and removed 3. to review Waiting for reviews labels Jul 15, 2022
@kesselb kesselb merged commit 52dc51c into master Jul 18, 2022
@kesselb kesselb deleted the s3-crt-bundle-download branch July 18, 2022 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants