feat(storage): add allow list check on S3 creation #858
Conversation
sihamais
force-pushed
the
feat/NCP-555/storage_allow_list
branch
from
d599d4e to
fddf2c5
Compare
storage/s3.go
Outdated
| }) | ||
| } | ||
|
|
||
| func NewS3(cfg S3Config, opts ...s3Opt) *S3 { | ||
| func NewS3(cfg S3Config, opts ...S3Opt) (*S3, error) { |
There was a problem hiding this comment.
Issue: Usually we don't want New functions to return error. It should only instantiate the object.
In addition, this will break retro-compatibility.
Suggestion: From #804 (comment) suggestion is to log something in run time.
But if you really prefer to throw an error, I'd see either:
- return it in runtime (e.g.
Get,Upload, ...) but not very practical in fact - create a dedicated helper to validate the list. But it need to add this check everywhere.
- keep it like this. In that case take look at the current implementations in our code base
There was a problem hiding this comment.
I changed the way it's handled. WDYT?
There was a problem hiding this comment.
Sorry for the delay and the change of mind, as we are in a context of configuration issue we should get an error as soon as possible. Hence the need to use an error in constructor (so your proposal in this commit: d599d4e...fddf2c5) 👍
From our implementation in code base (e.g. on database-core) we should make some change to fit the logic, i.e add a BackendGetter struct on boot of the related service.
Note: this will imply to make a major update of the package.
For the record: https://scalingo.slack.com/archives/CRC7L0XM2/p1714063053317099?thread_ts=1714058866.915739&cid=CRC7L0XM2
sihamais
force-pushed
the
feat/NCP-555/storage_allow_list
branch
8 times, most recently
from
03e6551 to
9ef694d
Compare
sihamais
force-pushed
the
feat/NCP-555/storage_allow_list
branch
from
9ef694d to
fc47dc8
Compare
| <<<<<<< HEAD | ||
| ======= | ||
| github.com/google/go-cmp v0.5.8 h1:e6P7q2lk1O+qJJb4BtCQXlK8vWEO8V1ZeuEdJNOqZyg= | ||
| github.com/google/go-cmp v0.5.8/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= | ||
| >>>>>>> 9ef694d (fix(s3): update go version) |
There was a problem hiding this comment.
There is still a remaining conflict here
There was a problem hiding this comment.
Oh sorry, missed that one
storage/s3.go
Outdated
| }) | ||
| } | ||
|
|
||
| func NewS3(cfg S3Config, opts ...s3Opt) *S3 { | ||
| func NewS3(cfg S3Config, opts ...S3Opt) (*S3, error) { |
There was a problem hiding this comment.
Sorry for the delay and the change of mind, as we are in a context of configuration issue we should get an error as soon as possible. Hence the need to use an error in constructor (so your proposal in this commit: d599d4e...fddf2c5) 👍
From our implementation in code base (e.g. on database-core) we should make some change to fit the logic, i.e add a BackendGetter struct on boot of the related service.
Note: this will imply to make a major update of the package.
For the record: https://scalingo.slack.com/archives/CRC7L0XM2/p1714063053317099?thread_ts=1714058866.915739&cid=CRC7L0XM2
| SigningRegion: cfg.Region, | ||
| }, nil | ||
| }) | ||
| if endpointIsAllowed(cfg.Endpoint) { |
There was a problem hiding this comment.
Suggestion(wording):
| if endpointIsAllowed(cfg.Endpoint) { | |
| if isEndpointAllowed(cfg.Endpoint) { |
|
@curzolapierre when you say "major update", what does this imply ? We're trying to keep a small scope in this change... |
|
As discussed with @leo-scalingo InfoSec will add the checks in the library and let IST update the projects during normal maintenance cycles |
CHANGELOG.mdNCP-555
Fix #804