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 AWS SSO authentication and option aws_home_dir (s3fs-fuse#1931) #2165

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

darioackermann
Copy link

Relevant Issue (if applicable)

#1931
already open MR: #1932

Details

  • Adds credentials grabbing from SSO

This is not very different from #1932
I have made the integration tests work (except for centos and rockylinux, for which the header is not available as package - what should we do there?) and adjusted stuff such that cppcheck doesn't complain, and resolved merge conflicts.

We could default that SSO support is not enabled automatically for centos/rockylinux builds. (and add a flag for enabling it).

@darioackermann darioackermann changed the title Add AWS SSO authentication and option aws_home_dir (s3fs-fuse#1932) Add AWS SSO authentication and option aws_home_dir (s3fs-fuse#1931) May 15, 2023
@ggtakec
Copy link
Member

ggtakec commented May 16, 2023

@darioackermann
I haven't been able to comment all of them yet, but let me ask you a quick question.

First of all, I don't think there is any processing when the SSO session token expires, but will it be updated outside of s3fs?
In general, is it enough that the cache file exists and is available to s3fs only for the duration of the token's validity period?
I am concerned that SSO(external authentication) generally has an expiration date.

The openssl code (SHA1) should be implemented in a separate file.
You will also need to implement it with gnuTLS and NSS.

I use nlohmann-json as my JSON parser, but I don't want to use it because it deviates from c++03 (I know it's old).
(exmaple, libyaml can also support c++03.)
However, I wonder if it should be included just to parse this SSO cache file.

Thanks for your help.

@darioackermann
Copy link
Author

No worries!

First of all, I don't think there is any processing when the SSO session token expires, but will it be updated outside of s3fs?

No, there is no processing when the token is expires. SSO tokens need to be renewed manually, by the user, by running aws sso login or aws sso login --profile target-sso-profile respectively. Renewing SSO token cannot be automated as the previously mentioned command opens a web browser where the user specifically needs to grant the authentication (even when the AWS session is still valid)

In general, is it enough that the cache file exists and is available to s3fs only for the duration of the token's validity period?

I see no other way of integrating the SSO feature otherwise, as there is no way to automatically renew the token. I have not yet tested what happens when the token expires, but the user would notice since the share is unavailable then (?)

I am concerned that SSO(external authentication) generally has an expiration date.

AWS SSO sessions last 8 hours I believe.

I use nlohmann-json as my JSON parser, but I don't want to use it because it deviates from c++03 (I know it's old).
(exmaple, libyaml can also support c++03.)
However, I wonder if it should be included just to parse this SSO cache file.

Generally, most of this MR is not my work, I have only resolved the conflicts and made most integration tests work (and cppcheck happy). I haven't changed anything in the logic.

Regarding the JSON header: I understand it raises the bar, I have only seen this comment in the other MR and assumed it was fine: https://github.com/s3fs-fuse/s3fs-fuse/pull/1932/files#r901044187 . If we're able to find a JSON parser that supports the c++03 standard that'd be an alternative, but a yaml parser won't work since the SSO tokens are stored in JSON format.

@ggtakec
Copy link
Member

ggtakec commented May 20, 2023

@darioackermann Thenks for your reply.

Regarding SSO support, I think @sqlbot's comment in the previous PR makes sense, and I also agree with him.

but the user would notice since the share is unavailable then (?)

Since s3fs usually runs on the server-side(background), I don't think the user will notice it easily if the token expires.
The behavior of s3fs and the file system at that time is presented to the user as errors such as file cannot be read, file cannot be written, file not found and etc.
I think it is difficult for the user to judge that SSO token is expired.

It would be useful if there was another application(i don't know if it exists) that would notice the SSO token update and automatically redo the SSO login.
But I think it only makes sense on the not server-side (personal desktops, laptops, etc.).

@gaul
Please give me your opinion.

Please let me discuss in this PR whether this AWS SSO support should be done or not.

NOTE

If we proceed with this PR(if we decide to support SSO), I would like you to make the following modifications to the source code.

  1. SHA1 code requires version of OpenSSL/gnuTLS/NSS.
    I could provide you with a sample implementing these.
  2. Don't use nlohmann-json, please change to code by libyaml or coding simple parser.
    libyaml is a library that can also parse json.(not yaml only)
    I'm sure I can provide a sample, either libyaml or a parser specifically for AWS files.

@darioackermann
Copy link
Author

darioackermann commented May 20, 2023

Yes: Auth by SSO is definitely not a server side feature and nothing that will work on an unmonitored system. But that's what EC2 roles are for for example.

Again: This is definitely an end-user feature: we'd like to follow AWS best practice and only generate IAM keys where really needed. We would want to avoid having IAM keys laying around for S3 access.

It would be useful if there was another application(i don't know if it exists) that would notice the SSO token update and automatically redo the SSO login.

You mean initiate the SSO login? There's always at least a manual click required (if the user is already logged in at AWS)

Automating the whole process would mean that we'd somehow have to store the credentials what is not the point of it..

libyaml is a library that can also parse json

Thanks for the clarification, got misled by the library name :))

@darioackermann
Copy link
Author

Second thought: it would be nice to have a possibility to inform the user about the expired SSO token, but I don't really see a way that can be used?

@ggtakec
Copy link
Member

ggtakec commented May 27, 2023

@darioackermann Thanks for your reply.
I understand the point of using IAM keys only when necessary.

However, the problem with expiring tokens is still that the cause of the error is hidden for the end user.

As you know, when the SSO token expires, s3fs tries to use that token and fails.
And since s3fs can only report the failure with errno, it cannot tell the party that performed the file manipulation operation that the token has expired.
Even if we can identify the cause from the s3fs log output, it is difficult to tell the end user.

It would be nice if there was some way to do it, but if this feature were built in, it would likely cause more confusion when the token expires than the inconvenience of not being able to use SSO token.
Our concerns are focused on this point.

@mgaunard
Copy link

If the credentials expire, it's the same as the drive failing or being hot-removed.

This feature is necessary for a lot of people, I see no harm in adding it. If you don't like the concept of temporary credentials, just use permanent ones instead.

@darioackermann
Copy link
Author

darioackermann commented Jun 28, 2023

I am a bit confused why we are discussing whether this shall be added or not.

  • This is an optional functionality, not enabled by default
  • People who don't want to use it (or don't need any SSO) will never stumble upon it
  • People who want to use it (those with SSO) will know that the credentials expire at some point and a manual refresh is needed.

As @mgaunard said, nobody is forced to use temporary SSO credentials. But, unlike permanent credentials (which AWS boos at), temporary SSO credentials are best practise.

I am happy to change the currently used libraries to the alternatives suggested, but only if there is some sort of guarantee that this feature makes it into s3fs-fuse.

@bemyak
Copy link

bemyak commented Oct 9, 2023

Hei,
I've tested the @darioackermann 's fork in combination with libs3fsawscred.so and it works!
A bit slowish maybe, but it's much better than having no possibility to authenticate with SSO at all.

Since s3fs usually runs on the server-side(background) …

This is not the case at least for me, I use s3fs-fuse to browse files on S3 from my computer. I think it's a valid use case, and kindly ask you to reconsider merging this, @ggtakec 🙏

@darioackermann
Copy link
Author

darioackermann commented Oct 9, 2023

There should be no need for using additional libraries when using the aws sso login --profile profile_name command.

I am still willing to make the needed adjustments, but haven't heard back that this PR will be merged eventually.

@bemyak
Copy link

bemyak commented Oct 10, 2023

@darioackermann If I run it without the plugin (after aws sso login), I get s3fs: Not found access key/secret key in passwd file. error, even though aws s3 ls works 🤷

Edit: The command I'm running is

s3fs <BUCKET_NAME> ./mountdir/ -o use_cache=true

@kuza55
Copy link

kuza55 commented Nov 21, 2023

Hi,

I wanted to ping this as another user invested in this as an option.

I want to use s3fs on my development machine and keep using SSO for auth.

I understand that FUSE cannot report detailed errors, but you get used to having to reauth every morning and that errors you experience before then have a clear cause.

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.

None yet

6 participants