codenotary / immudb

immudb - immutable database based on zero trust, SQL/Key-Value/Document model, tamperproof, data change history
https://immudb.io
Other
8.62k stars 343 forks source link

PR to handle the AWS v4 signing by using IAM Role #2002

Closed lorepas closed 2 months ago

lorepas commented 3 months ago

Fixed a bug encountered when using assume role to connect immudb to an S3 bucket. In this PR I added the manage of the SessionToken and I used the X-Amz-Security-Token header as specified here: https://docs.aws.amazon.com/AmazonS3/latest/userguide/RESTAuthentication.html

lorepas commented 2 months ago

@jeroiraz have you take a look into this fix?

ostafen commented 2 months ago

Hi, @lorepas, first of all thanks for the fix. Could you please align your branch with immudb's latest master commit? Also, I've seen that your PR contains a lot of extra tabs (maybe due to your formatter config) that make difficult to isolate the exact changes of the PR. Thanks!

lorepas commented 2 months ago

Hi, @lorepas, first of all thanks for the fix. Could you please align your branch with immudb's latest master commit? Also, I've seen that your PR contains a lot of extra tabs (maybe due to your formatter config) that make difficult to isolate the exact changes of the PR. Thanks!

Hi @ostafen and thank you for your reply! Now I refactored the code, so the changes you can see now are the ones made for the fix. For these changes I started from the latest opensource version, the 1.9DOM.2, for this reason I would put these changes into such version. Do you think is that possible?

ostafen commented 2 months ago

@lorepas: latest version is now 1.9.4. Any specific reason you are using 1.9DOM.2?

lorepas commented 2 months ago

@lorepas: latest version is now 1.9.4. Any specific reason you are using 1.9DOM.2?

@ostafen because from the change of license made by immudb I understand that the latest opensource was 1.9DOM.2. All the ones after such version are licensed BSL. Is that right?

ostafen commented 2 months ago

@lorepas The license change doesn't change the fact that it's open source or the project cannot be used by anyone. The only real restriction affects the commercial usage in a very limited range. Thus, allowing PRs to outdated versions wouldn't make much sense and would confuse users and developers.

lorepas commented 2 months ago

@lorepas The license change doesn't change the fact that it's open source or the project cannot be used by anyone. The only real restriction affects the commercial usage in a very limited range. Thus, allowing PRs to outdated versions wouldn't make much sense and would confuse users and developers.

@ostafen yes, it's clear since the code is public all can contribute on that and for this reason it's opensource. I changed target branch to the master, however I've one last question: which are the limits on production use case about the use of immudb with BSL?

ostafen commented 2 months ago

@lorepas: can you solve conflicts?

lorepas commented 2 months ago

@ostafen conflicts solved!

coveralls commented 2 months ago

Coverage Status

coverage: 89.457% (-0.01%) from 89.469% when pulling 10697671dd7e97e412972cc9f3b18de0b821bf80 on lorepas:lp-aws-signed-request-with-role into 76cd08a0ec757a3dc498d584799d15df1aa5a9ef on codenotary:master.

ostafen commented 2 months ago

It looks like code scanning complains about logging storage here, since it could reveal secretKey. @lorepas, would you mind changing such line to:

log.Printf("Opening remote storage at %s", remotePath)

It would be also great if you could squash your commits into one, in order to keep history clean. Thanks!

lorepas commented 2 months ago

@ostafen I changed the line suggested by you

ostafen commented 2 months ago

Some related test is failing:

image

lorepas commented 2 months ago

@ostafen added the needed flag also in related test.

ostafen commented 2 months ago

@lorepas: please, make sure all tests are passing: TestSignatureV4 is still failing.

lorepas commented 2 months ago

@ostafen Now I set the header x-amz-security-token only if role is enabled. I fixed also the test by eliminating such header since the test doesn't perform temporary authentication. I think now the test should work.

ostafen commented 2 months ago

@lorepas: Thanks for fixing the test. Before merging, could you please squash your commits into one so to keep history clean?

lorepas commented 2 months ago

@ostafen I tried to squash my commits but currently they increase in number by taking all the ones from my first and now. Can you do the squash while you're accepting the PR? Otherwise I accept hint to understand how to do such operation.

ostafen commented 2 months ago

@lorepas: I cannot merge the PR, because not all the commits are signed. I suggest to squash all the commits into one, signed commit. Regarding squashing your commits, read this.

lorepas commented 2 months ago

Close in favor of #2010