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 settings to allow enabling TLS for grpc services #3332

Merged
merged 2 commits into from
Oct 19, 2022

Conversation

rhafer
Copy link
Contributor

@rhafer rhafer commented Oct 10, 2022

For grpc services TLS can be enabled per revad instance in the new
[grpc.tls_settings] section:

[grpc.tls_settings]`
enabled = true
certificate = "<path/to/cert.pem>"
key = "<path/to/key.pem>"

If certificate or key are empty a temporary self-signed certificate
is generated upon startup. The default for enabled is currently
"false", i.e. transport security is disabled.

The configuration of grpc clients is currently only possible in the
[shared.grpc_client_options] section (not on a per client level):

[shared.grpc_client_options]
tls_mode = "on"
tls_cacert = "</path/to/cafile.pem>"

tls_mode has 3 possible values. off (the default) disables transport
security for the clients. insecure allows to use transport security,
but disables certificate verification (to be used with the autogenerated
self-signed certificates). on enables transport security.

tls_cacert is only relevant when tls_mode is set to on and allows
specifying a ca file/bundle to be used for verifying server
certificates.

For config examples see the toml files in tests/oc-integration-tests/local

@rhafer
Copy link
Contributor Author

rhafer commented Oct 11, 2022

The cs3api-validator tests are still failing with TLS enabled, because the validator doesn't know the new TLS related flags yet. It's a bit of a hen and egg issue. I guess it's easiest to keep TLS disabled for the tests and enable it after this change got merged (and the validator is updated to use the options).

@lgtm-com
Copy link

lgtm-com bot commented Oct 12, 2022

This pull request introduces 1 alert when merging 8c24f35 into d908525 - view on LGTM.com

new alerts:

  • 1 for Disabled TLS certificate check

@cs3org cs3org deleted a comment from lgtm-com bot Oct 13, 2022
@cs3org cs3org deleted a comment from lgtm-com bot Oct 13, 2022
@cs3org cs3org deleted a comment from lgtm-com bot Oct 13, 2022
@cs3org cs3org deleted a comment from lgtm-com bot Oct 13, 2022
@cs3org cs3org deleted a comment from lgtm-com bot Oct 13, 2022

```toml
[shared.grpc_client_options]
tls_mode = "on"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@micbar @C0rby Different to my initial approach, where I just had a tls_insecure boolean this is introducing a third state for the client's tls setting which allows to completely turn off TLS (e.g. for debugging, or for isolated deployments where TLS is not really needed).

This is somewhat different to the way we configure TLS clients in other places, but I think we need this kind of switch. Are you ok with this, or I go back to the tls_insecure=true/false setting without allowing to turn TLS off.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think it could make sense to turn TLS off in certain situations.
This means we need to adjust those other places but we can do that in separate PRs.

@rhafer rhafer marked this pull request as ready for review October 13, 2022 08:18
@lgtm-com
Copy link

lgtm-com bot commented Oct 13, 2022

This pull request introduces 1 alert when merging 1f72786 into 11cc78a - view on LGTM.com

new alerts:

  • 1 for Disabled TLS certificate check

@lgtm-com
Copy link

lgtm-com bot commented Oct 13, 2022

This pull request introduces 1 alert when merging b8944a3 into 11cc78a - view on LGTM.com

new alerts:

  • 1 for Disabled TLS certificate check

changelog/unreleased/grpc-tls-settings.md Outdated Show resolved Hide resolved
For grpc services TLS can be enabled per revad instance in the new
`[grpc.tls_settings]` section:

```
[grpc.tls_settings]`
enabled = true
certificate = "<path/to/cert.pem>"
key = "<path/to/key.pem>"
```

If `certificate` or `key` are empty a temporary self-signed certificate
is generated upon startup. The default for `enabled` is currently
`"false"`, i.e. transport security is disabled.

The configuration of grpc clients is currently only possible in the
`[shared.grpc_client_options]` section (not on a per client level):

```
[shared.grpc_client_options]
tls_mode = "on"
tls_cacert = "</path/to/cafile.pem>"

```

`tls_mode` has 3 possible values. `off` (the default) disables transport
security for the clients. `insecure` allows to use transport security,
but disables certificate verification (to be used with the autogenerated
self-signed certificates). `on` enables transport security.

`tls_cacert` is only relevant when `tls_mode` is set to `on` and allows
specifying a ca file/bundle to be used for verifying server
certificates.
This is a (temporary) workaround for allowing to separately configure the
tls mode for the permissions client used by the decomposedfs storage. This
is needed for being able to use the ocis settings service as the permission
service untils ocis' go-micro based services are adapted for providing TLS
as well.
@lgtm-com
Copy link

lgtm-com bot commented Oct 19, 2022

This pull request introduces 1 alert when merging e3ee706 into 203cc6b - view on LGTM.com

new alerts:

  • 1 for Disabled TLS certificate check

@rhafer rhafer merged commit df0a189 into cs3org:edge Oct 19, 2022
rhafer added a commit to rhafer/cs3api-validator that referenced this pull request Oct 19, 2022
aduffeck pushed a commit to owncloud/cs3api-validator that referenced this pull request Jan 10, 2024
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.

2 participants