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

Add SSL configuration for RGW #355

Merged
merged 41 commits into from
Aug 20, 2024

Conversation

marceloneppel
Copy link
Member

@marceloneppel marceloneppel commented Jun 3, 2024

Description

Some tools, like pgBackRest, can currently only interact with S3-compatible storages if they work with SSL/TLS. This PR adds the possibility of enabling RadosGW with SSL/TLS enabled.

The main idea is to use the PostgreSQL charms with MicroCeph so users can do backups through pgBackRest in bucket without a cloud service subscription.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

To test, I used the following steps:

  1. Generate some SSL files:
sudo openssl genrsa -out /var/snap/microceph/common/ca.key 2048
sudo openssl req -x509 -new -nodes -key /var/snap/microceph/common/ca.key -days 1024 -out /var/snap/microceph/common/ca.crt -outform PEM
sudo openssl genrsa -out /var/snap/microceph/common/server.key 2048
sudo openssl req -new -key /var/snap/microceph/common/server.key -out /var/snap/microceph/common/server.csr
sudo nano /var/snap/microceph/common/extfile.cnf # and put the following content: subjectAltName = DNS:localhost
sudo openssl x509 -req -in /var/snap/microceph/common/server.csr -CA /var/snap/microceph/common/ca.crt -CAkey /var/snap/microceph/common/ca.key -CAcreateserial -out /var/snap/microceph/common/server.crt -days 365 -extfile /var/snap/microceph/common/extfile.cnf
  1. Then bootstrap the MicroCeph cluster, enable the RadosGW service with SSL enabled and create a user:
sudo microceph cluster bootstrap
sudo microceph disk add loop,4G,3
sudo microceph enable rgw --ssl-certificate=/var/snap/microceph/common/server.crt --ssl-private-key=/var/snap/microceph/common/server.key
sudo microceph.radosgw-admin user create --uid test --display-name test
  1. To finish, test the access by creating a bucket:
aws configure # to configure the credentials from RadosGW.
AWS_CA_BUNDLE=/var/snap/microceph/common/ca.crt aws --endpoint-url=https://localhost s3 mb s3://test --region ""

Contributor's Checklist

Please check that you have:

  • self-reviewed the code in this PR.
  • added code comments, particularly in hard-to-understand areas.
  • updated the user documentation with corresponding changes.
  • added tests to verify effectiveness of this change.

Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Copy link
Contributor

@UtkarshBhatthere UtkarshBhatthere left a comment

Choose a reason for hiding this comment

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

one nit, overall LGTM

Copy link
Collaborator

@sabaini sabaini left a comment

Choose a reason for hiding this comment

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

Hey @marceloneppel thanks for this welcome contribution!

I'm wondering about two things regarding parametrization.

One is relatively minor thing, aiui with SSL key material present we'd configure both http/https. Ideally we would have a way to turn off non-SSL service if we configure SSL key material -- maybe the logic could be to configure https if SSL key material is provided, and http if there's no key material. And only configure both if both ports are explicitly provided.

The other is around that key material. In your PR the user has to provide file paths to the key material. However due to snap confinement there's a limited number of places the services can actually read data from, and users would see failing services if the ssl files are not in a suitable place.
I'd suggest to check in the CLI part of the code that the file is readable for the snap so users get an early warning, and document the constraints around this.
Alternatively, the code could be changed so that the key material itself is used as a parameter (instead of the file names).

It would also be great to have functional tests for this feature.

Thanks again!

@marceloneppel
Copy link
Member Author

Hey @marceloneppel thanks for this welcome contribution!

I'm wondering about two things regarding parametrization.

One is relatively minor thing, aiui with SSL key material present we'd configure both http/https. Ideally we would have a way to turn off non-SSL service if we configure SSL key material -- maybe the logic could be to configure https if SSL key material is provided, and http if there's no key material. And only configure both if both ports are explicitly provided.

The other is around that key material. In your PR the user has to provide file paths to the key material. However due to snap confinement there's a limited number of places the services can actually read data from, and users would see failing services if the ssl files are not in a suitable place. I'd suggest to check in the CLI part of the code that the file is readable for the snap so users get an early warning, and document the constraints around this. Alternatively, the code could be changed so that the key material itself is used as a parameter (instead of the file names).

It would also be great to have functional tests for this feature.

Thanks again!

Hi, @sabaini! Thanks for the feedback. I'm going to work on those updates.

Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Copy link
Contributor

@lmlg lmlg left a comment

Choose a reason for hiding this comment

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

lgtm, great stuff :)

Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Copy link
Member Author

Hey @marceloneppel thanks for this welcome contribution!

I'm wondering about two things regarding parametrization.

One is relatively minor thing, aiui with SSL key material present we'd configure both http/https. Ideally we would have a way to turn off non-SSL service if we configure SSL key material -- maybe the logic could be to configure https if SSL key material is provided, and http if there's no key material. And only configure both if both ports are explicitly provided.

The other is around that key material. In your PR the user has to provide file paths to the key material. However due to snap confinement there's a limited number of places the services can actually read data from, and users would see failing services if the ssl files are not in a suitable place. I'd suggest to check in the CLI part of the code that the file is readable for the snap so users get an early warning, and document the constraints around this. Alternatively, the code could be changed so that the key material itself is used as a parameter (instead of the file names).

It would also be great to have functional tests for this feature.

Thanks again!

Hi @sabaini! I fixed the issues I was facing and updated this PR based on your comments.

The code is now turning off the HTTP service if no HTTP port is specified and the SSL material is specified. It's possible to turn on both HTTP and HTTPS services if both the HTTP port and the SSL material are specified.

Also, to avoid file permission issues, the new parameters were changed from file paths to the SSL material:

sudo microceph enable rgw --ssl-certificate="$(sudo base64 -w0 ~/server.crt)" --ssl-private-key="$(sudo base64 -w0 ~/server.key)"

What's missing are the functional tests. I'll add them next week.

Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel
Copy link
Member Author

@sabaini, I added a functional test through 5f60871.

@marceloneppel marceloneppel requested a review from sabaini August 16, 2024 21:14
Copy link
Collaborator

@sabaini sabaini left a comment

Choose a reason for hiding this comment

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

Thank you @marceloneppel, very nice!
Some commentary / Q inline

microceph/ceph/rgw.go Outdated Show resolved Hide resolved
microceph/ceph/rgw.go Outdated Show resolved Hide resolved
microceph/ceph/rgw.go Outdated Show resolved Hide resolved
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel marceloneppel requested a review from sabaini August 19, 2024 12:35
Copy link
Collaborator

@sabaini sabaini left a comment

Choose a reason for hiding this comment

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

lgtm, thank you!

@sabaini
Copy link
Collaborator

sabaini commented Aug 19, 2024

@UtkarshBhatthere think you had requested changes too -- please re-review?

Copy link
Contributor

@UtkarshBhatthere UtkarshBhatthere left a comment

Choose a reason for hiding this comment

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

LGTM, 👍🏼 for good testing coverage.

@UtkarshBhatthere UtkarshBhatthere merged commit f85ee55 into canonical:main Aug 20, 2024
14 checks passed
@UtkarshBhatthere
Copy link
Contributor

@marceloneppel as a programmer how was your experience extending the RGW service capabilities in MC ? Asking specifically about using the service placement interface. Was is easy to understand ?

@marceloneppel
Copy link
Member Author

@marceloneppel as a programmer how was your experience extending the RGW service capabilities in MC ? Asking specifically about using the service placement interface. Was is easy to understand ?

Hi, @UtkarshBhatthere! It was easy to understand, specifically the placement interface. The part where I struggled a little bit was related to the RGW configuration, not MC.

@UtkarshBhatthere
Copy link
Contributor

Great :D

@maximsachs
Copy link

Hey @marceloneppel thanks for this welcome contribution!
I'm wondering about two things regarding parametrization.
One is relatively minor thing, aiui with SSL key material present we'd configure both http/https. Ideally we would have a way to turn off non-SSL service if we configure SSL key material -- maybe the logic could be to configure https if SSL key material is provided, and http if there's no key material. And only configure both if both ports are explicitly provided.
The other is around that key material. In your PR the user has to provide file paths to the key material. However due to snap confinement there's a limited number of places the services can actually read data from, and users would see failing services if the ssl files are not in a suitable place. I'd suggest to check in the CLI part of the code that the file is readable for the snap so users get an early warning, and document the constraints around this. Alternatively, the code could be changed so that the key material itself is used as a parameter (instead of the file names).
It would also be great to have functional tests for this feature.
Thanks again!

Hi @sabaini! I fixed the issues I was facing and updated this PR based on your comments.

The code is now turning off the HTTP service if no HTTP port is specified and the SSL material is specified. It's possible to turn on both HTTP and HTTPS services if both the HTTP port and the SSL material are specified.

Also, to avoid file permission issues, the new parameters were changed from file paths to the SSL material:

sudo microceph enable rgw --ssl-certificate="$(sudo base64 -w0 ~/server.crt)" --ssl-private-key="$(sudo base64 -w0 ~/server.key)"

What's missing are the functional tests. I'll add them next week.

Hi @marceloneppel I spent the last two days frustrated trying to find how to add SSL to the rados gateway until I found your PR! Awesome work! A thing I am still confused about, is since you switched from filepath to base64 encoded cert and key, when I update the keys how is the reloading intended to work?

I normally set up LetsEncrypt to automatically renew certificates, and would then expect to restart the gateway service with: sudo systemctl restart snap.microceph.rgw.service. But since the cert/key are effectively "hardcoded" this would not lead to a restart with renewed certificates.

So for now I would have to disable the rgw and re-enable it again with the updated certs in a deploy-hook on certbot, but that doesn't feel very clean.

I would love a way to simply swap out certs for LetsEncrypt compatibility, or if you have ideas on how this can be achieved without disabling the rgw that would be appreciated.

@UtkarshBhatthere
Copy link
Contributor

@maximsachs thanks for sharing your feedback on the user experience. I really appreciate it. May I request you to create a GitHub issue that expresses your expected UX ? This helps us keep track of the the task.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants