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

Use TLS protocol #200

Merged
merged 2 commits into from
Apr 27, 2024
Merged

Use TLS protocol #200

merged 2 commits into from
Apr 27, 2024

Conversation

tatiana-nspcc
Copy link
Contributor

Close #170.

@tatiana-nspcc tatiana-nspcc force-pushed the 170-tls-fix branch 2 times, most recently from a2b012e to 9998d44 Compare April 19, 2024 16:04
Copy link

codecov bot commented Apr 22, 2024

Codecov Report

Attention: Patch coverage is 7.89474% with 105 lines in your changes are missing coverage. Please review.

Project coverage is 22.14%. Comparing base (52e9a12) to head (c4f9507).
Report is 3 commits behind head on master.

Files Patch % Lines
cmd/neofs-rest-gw/config.go 13.63% 49 Missing and 8 partials ⚠️
cmd/neofs-rest-gw/main.go 0.00% 41 Missing ⚠️
cmd/neofs-rest-gw/server.go 0.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #200      +/-   ##
==========================================
- Coverage   22.88%   22.14%   -0.74%     
==========================================
  Files          15       16       +1     
  Lines        2836     2894      +58     
==========================================
- Hits          649      641       -8     
- Misses       2079     2137      +58     
- Partials      108      116       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

cmd/neofs-rest-gw/main.go Outdated Show resolved Hide resolved
@@ -428,7 +429,7 @@ type ServerConfig struct {
}

const (
FlagScheme = "scheme"
FlagScheme = "schemes"
Copy link
Member

@roman-khimov roman-khimov Apr 22, 2024

Choose a reason for hiding this comment

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

This needs some documentation in CHANGELOG, btw. "Updating from 0.8.3".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I revised the tls-* section in config.yaml and figured out that:

  1. tls-listen-limit is not used. Should we remove it?
  2. It was stated in "Updating from 0.7.2":
Notice that server.scheme setting is an array, it was not enforced in the
previous version (it worked fine with a string), but 0.8.0 will not work with
this incorrect configuration, so please check your configurations.

However, it worked throughout the 0.8.* versions. Moreover, the HTTP server starts regardless of the scheme/schemes parameter. If we want to add HTTPS, we can set it as [https] or [http, https]. The value of it only affects the Swagger docs. Is this OK? Or do we want to stop the HTTP server from starting if it's not intentionally configured?

Copy link
Member

@roman-khimov roman-khimov Apr 23, 2024

Choose a reason for hiding this comment

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

tls-listen-limit is not used. Should we remove it?

No other gateway has it, let's drop for now.

Moreover, the HTTP server starts regardless of the scheme/schemes parameter

How about dropping it completely? @smallhive

external-address then would have to have the scheme in it, but it's OK

external-address: https://something.somewhere

instead of

schemes: [ https ]
external-address: something.somewhere

This can be explained in "upgrading from".

Copy link
Contributor Author

@tatiana-nspcc tatiana-nspcc Apr 23, 2024

Choose a reason for hiding this comment

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

How about dropping it completely? @smallhive

external-address then would have to have the scheme in it, but it's OK

external-address: https://something.somewhere

In this case, external-address can be a list because the service can serve both HTTP and HTTPS. We should rename it to external-addresses, I assume. Also, I need to add another flag that indicates we want to serve the TLS protocol, like tls-enabled.

Copy link
Member

Choose a reason for hiding this comment

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

external-addresses

OK.

tls-enabled

S3:

    tls:
      enabled: true
      cert_file: /path/to/cert
      key_file: /path/to/key

node:

    tls:
      enabled: true  # use TLS for a gRPC connection (min version is TLS 1.2)
      certificate: /path/to/cert  # path to TLS certificate
      key: /path/to/key  # path to TLS key

NeoGo:

// BasicService is used as a simple base for node services like Pprof, RPC or
// Prometheus monitoring.
type BasicService struct {
        Enabled bool `yaml:"Enabled"`
        // Addresses holds the list of bind addresses in the form of "address:port".
        Addresses []string `yaml:"Addresses"`
}

        // TLS describes SSL/TLS configuration.
        TLS struct {
                BasicService `yaml:",inline"`
                CertFile     string `yaml:"CertFile"`
                KeyFile      string `yaml:"KeyFile"`
        }

There are differences, but they're minor. REST however is radically different. But it shouldn't be. Let's make it more like S3 or node but without introducing multi-address.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@tatiana-nspcc tatiana-nspcc force-pushed the 170-tls-fix branch 2 times, most recently from 6706f38 to 564a4f8 Compare April 23, 2024 11:35
CHANGELOG.md Outdated
@@ -4,6 +4,15 @@ This document outlines major changes between releases.

## [Unreleased]

### Updating from 0.8.4
Copy link
Member

Choose a reason for hiding this comment

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

0.8.3! It's my mistake, we had so many 0.8.x that I started to think 0.8.4 is real. In fact it's not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Configuration parameters in the `server` section are reorganized. For example,
`server.schema` and `tls-listen-limit` are removed, and some others are moved
inside the array `server.endpoints`.
The documentation in `gate-configuration.md` is updated.
Flags in the command arguments are changed.

Signed-off-by: Tatiana Nesterenko <[email protected]>
@tatiana-nspcc
Copy link
Contributor Author

I've made many changes; please conduct a thorough review again.

Additionally, I would like to point out that:

  1. I did not add validation for config parameters in the server.endpoints as I don't see the necessity, but I can do it if needed.
  2. In the HTTP server, there is an option to set parameters (keep-alive, read-timeout, write-timeout), but they were not previously used at server start, and I have preserved this behaviour.
  3. I did not delve into parameters not related to the server and TLS, but I am concerned that possibly not everything is being used or is inaccurately reflected in gate-configuration.md. I recommend checking this separately and creating an issue for updating.

@roman-khimov roman-khimov merged commit f024e79 into master Apr 27, 2024
11 of 14 checks passed
@roman-khimov roman-khimov deleted the 170-tls-fix branch April 27, 2024 13:17
tatiana-nspcc added a commit to nspcc-dev/neofs-testcases that referenced this pull request Apr 27, 2024
The configuration structure of the REST gateway has changed:
nspcc-dev/neofs-rest-gw#200

Close #791.

Signed-off-by: Tatiana Nesterenko <[email protected]>
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.

TLSListenAddress doesn't work
3 participants