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 FreeBSD support (#188) #768

Closed
wants to merge 1 commit into from
Closed

Conversation

cemeyer
Copy link

@cemeyer cemeyer commented Dec 3, 2019

As discussed in #768, I renamed credCacheInternal_linux.go to
credCacheGnomeKeyring_unix.go with the intention to provide a generic
libsecret (née GNOME Keyring) credential cache implementation. Continue to
build this implementation in the (linux && se_integration) target, but
additionally build this implementation for the FreeBSD target, which lacks
an in-kernel credential cache service.

Rename credCacheGnomeKeyring_linux.go to credCacheGnomeKeyringBindings_unix.go
to reflect that this file contains generic cgo wrappers of the GnomeKeyring
(libsecret) API (neither Linux-specific, nor an implementation of the
CredCache interface). Add wrappers for secret_password_store() (add) and
secret_password_clear() (delete).

The struct CredCacheInternalIntegration was mechanically renamed to
CredCacheGnomeKeyring to more closely match its new use. Additionally,
trivial implementations of removeCachedTokenInternal() and
saveTokenInternal() were defined for CredCacheGnomeKeyring.

On Linux (credCache_linux), an alias to the old name is defined for the
se_integration target. On FreeBSD, the type names CredCache and
CredCacheInternalIntegration are both aliases of CredCacheGnomeKeyring.

This means the FreeBSD port pulls in cgo and libsecret. But it does not add
any new dependency for other platforms.

More trivially, rename mmf_linux to mmf_unix (not Linux-specific) and add
+build rules to enable it on Linux and FreeBSD. A straightforward
writeThroughFile_freebsd is implemented (thought Go makes this hard on us by
not providing a nice syscall wrapper for fallocate).

I didn't try to run the test suite yet.

@msftclas
Copy link

msftclas commented Dec 3, 2019

CLA assistant check
All CLA requirements met.

@JohnRusk
Copy link
Member

JohnRusk commented Dec 3, 2019

Thanks @cemeyer . I'll tag this for release 10.4 (At this stage I don't think it belongs in the earlier 10.3.3 bug-fix release that we are planning).

@JohnRusk JohnRusk added this to the 10.4.0 milestone Dec 3, 2019
@cemeyer
Copy link
Author

cemeyer commented Dec 3, 2019

Thanks @JohnRusk . That sounds great to me.

@JohnRusk
Copy link
Member

@cemeyer one question about the cred cache implementation here. I have't read the code yet, but you mention that it runs in memory. How does that work with the normal sequence of two separate calls:
azcopy login followed by azcopy copy\sync?

@cemeyer
Copy link
Author

cemeyer commented Dec 11, 2019

I don't think it works for that scenario. :-)

It seems like the Python az CLI framework fetches and renews its own authentication tokens, stored in ~/.azure/accessTokens.json. It never invokes azcopy login; only azcopy copy, azcopy remove, or azcopy sync. It looks like it can pass credentials to azcopy via environment variable AZCOPY_OAUTH_TOKEN_INFO.

Empirically, I've had no credential issues using az storage copy to invoke azcopy. (And in fact, my only interest in azcopy is that az storage copy requires it and az storage copy is the only way to upload a disk image to Azure...)

So, given how azcopy's credcache is used, it might be equally useful to just provide do-nothing stubs instead of the memory cache.

@JohnRusk
Copy link
Member

Ah, I see, I didn't realize that az storage copy was in the picture here.

@JohnRusk
Copy link
Member

BTW, re uploading a disk image to Azure, the part of the work that AzCopy does is basically just uploading a page blob to a special URL. The main difference between that and a normal page blob upload is that you can't create the page blob because it already exists. There's nothing particularly special about AzCopy's part of the managed disk upload process. The only "special" bit is the bit before uploading where Azure set up the managed disk in a ready-to-upload state, and supplies the SAS URL for it. I don't know the details of how that bit works. It's not in AzCopy..

Copy link
Member

@JohnRusk JohnRusk left a comment

Choose a reason for hiding this comment

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

Looks good. My feedback just relates to the comments.

common/credCache_bsd.go Outdated Show resolved Hide resolved
common/credCache_bsd.go Outdated Show resolved Hide resolved
common/writeThroughFile_freebsd.go Outdated Show resolved Hide resolved
@cemeyer
Copy link
Author

cemeyer commented Dec 17, 2019

Updated the comments and re-pushed.

Copy link
Member

@adreed-msft adreed-msft left a comment

Choose a reason for hiding this comment

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

:shipit:

@JohnRusk
Copy link
Member

@cemeyer I'm finally getting back to this, as we work toward the 10.4 release.

You mentioned that the new code is just to allow the token to be passed in with an environment variable. In which case, I have a question: would it make more sense if all the methods of credCache_bsd.go were just no-ops (or perhaps even panic("not supported"))?

Due to the way this works, I believe the methods will never actually be called (but I might be mistaken)

@cemeyer
Copy link
Author

cemeyer commented Mar 11, 2020

In which case, I have a question: would it make more sense if all the methods of credCache_bsd.go were just no-ops (or perhaps even panic("not supported"))?

I suspect you're right, and it would make sense. (But I have not tested that approach.)

@JohnRusk
Copy link
Member

Does anyone know if libsecret is supported on FreeBSD? If so, that might give a more full-featured login experience than this.....

@JohnRusk
Copy link
Member

(I only understood this week that we have conditional compilation stuff in the codebase to support libscecret for the purpose of running on OS's where the "normal" keyrings aren't there)

@cemeyer
Copy link
Author

cemeyer commented Mar 12, 2020

@JohnRusk We do appear to have a FreeBSD libsecret port (package) available, so that may be a better option. (I had not heard of that until just now, either. :-))

https://www.freshports.org/security/libsecret/

@JohnRusk
Copy link
Member

Take a look, if you like, at the file azure-storage-azcopy\common\credCacheGnomeKeyring_linux.go. At the moment, it's not included in the public builds (for the reason explained in credCacheGnomeKeyringShim_linux.go), but it might be preferrable if it was included in FreeBSD builds.

In terms of good security practice, it would be better I think to move away from the environment variable that this PR depends on, to that keyring (not just for Free BSD, but in general).

Anyway, these are just top of mind thoughts, based on what I learned this week. Interested in your thoughts.

As discussed in Azure#768, I renamed credCacheInternal_linux.go to
credCacheGnomeKeyring_unix.go with the intention to provide a generic
libsecret (née GNOME Keyring) credential cache implementation.  Continue to
build this implementation in the (linux && se_integration) target, but
additionally build this implementation for the FreeBSD target, which lacks
an in-kernel credential cache service.

Rename credCacheGnomeKeyring_linux.go to credCacheGnomeKeyringBindings_unix.go
to reflect that this file contains generic cgo wrappers of the GnomeKeyring
(libsecret) API (neither Linux-specific, nor an implementation of the
CredCache interface).  Add wrappers for secret_password_store() (add) and
secret_password_clear() (delete).

The struct CredCacheInternalIntegration was mechanically renamed to
CredCacheGnomeKeyring to more closely match its new use.  Additionally,
trivial implementations of removeCachedTokenInternal() and
saveTokenInternal() were defined for CredCacheGnomeKeyring.

On Linux (credCache_linux), an alias to the old name is defined for the
se_integration target.  On FreeBSD, the type names CredCache and
CredCacheInternalIntegration are both aliases of CredCacheGnomeKeyring.

This means the FreeBSD port pulls in cgo and libsecret.  But it does not add
any new dependency for other platforms.

More trivially, rename mmf_linux to mmf_unix (not Linux-specific) and add
+build rules to enable it on Linux and FreeBSD.  A straightforward
writeThroughFile_freebsd is implemented (thought Go makes this hard on us by
not providing a nice syscall wrapper for fallocate).

I didn't try to run the test suite yet.
@cemeyer
Copy link
Author

cemeyer commented Mar 15, 2020

I went ahead with your proposal to integrate with libsecret / gnome-keyring-daemon. For now, it is only used on FreeBSD and linux+se_integration (where it was already used) to minimize impact/risk to other targets.

With that implementation, I was able to azcopy login; azcopy cp ..., as expected. (Or, well, mostly expected. Performance is worse than expected / observed on earlier versions of azcopy, but I doubt that can be ascribed to the credential store implementation. Most likely it's just residential ISP fluctuation / additional load thanks to COVID.)

Some design decisions worth being aware of:

  1. I opted for the "default" credential storage lifetime, which is infinity, I think. The other option is "session." It looks like Windows' credCache uses something approximating infinity (files) while the Linux credCache uses something more approximating session. I might be wrong on both counts, I'm unfamiliar with those interfaces. It would be easy to throw the switch to "session" if that fits expectations better.
  2. The existing cgo wrappers for libsecret used the C string based interface. I think that's fine for azcopy — we're just storing JSON blobs, which are inherently C strings (nul-safe). Newer versions of libsecret (0.19+) provide a binary blob storage API, and I think that'd be preferable. However, the version of libsecret in FreeBSD ports is 0.18.x and lacks this feature, so I just left a comment documenting the decision and continued using the C string interfaces for now.

I'll push a new patch in a moment. Thanks for the feedback and guidance.

@cemeyer cemeyer changed the title Add basic FreeBSD support (#188) Add FreeBSD support (#188) Mar 15, 2020
@cemeyer
Copy link
Author

cemeyer commented Mar 20, 2020

Is there anything I can do to help facilitate merging this, or is it just a matter of eyeballs, priorities, and time? I totally understand there may be more pressing issues and/or slowdowns as we figure out how to work during the various COVID-19 shutdowns. Thanks!

@cemeyer cemeyer mentioned this pull request Mar 21, 2020
@JohnRusk
Copy link
Member

Eyeballs and time are the issue. @zezha-msft is planning to follow up with you

@adreed-msft adreed-msft modified the milestones: 10.4.0, 10.5.0 Apr 10, 2020
@zezha-msft
Copy link
Contributor

Hi @cemeyer, I really apologize for the much delayed reply. I'm afraid that we have rework part of this PR.

Recently, we've received many feedbacks (from multiple sources) that concerns the difficulties in getting the keyring to work on different platforms/environments. The solutions are not always trivial/workable, and it's becoming a bigger problem than we first realized.

The Team has proposed an alternative solution that has that been floated around for a while now, and that is to allow users to login and execute a copy operation all in one command. This means that the tokens can be safely stored in memory and removed appropriately once the copy command exits. More importantly, this solution does not have any kind of platform dependence, and will allow FreeBSD users to use the login feature while we don't have to use the libsecret port.

While we work on designing this new functionality, please don't hesitate if you have any thoughts/feedbacks on the proposed approach.

@cemeyer
Copy link
Author

cemeyer commented Jun 3, 2020

Hi @zezha-msft , that approach sounds fine to me. It seems fundamentally similar to the approach I took in this PR back in December: 463327d .

You've marked this PR as "Changes requested," but I don't think there's anything actionable on my end (at least, until the new functionality is designed/implemented). Is that right?

Thanks for the update.

@zezha-msft
Copy link
Contributor

Hi @cemeyer, yes, we'll follow up soon. Thanks!

@cemeyer
Copy link
Author

cemeyer commented Jun 4, 2020

Thanks!

@zezha-msft zezha-msft added pr-on-hold Use on PRs to indicate ones that shouldn't be reviewed right now because they're blocked on somethin and removed needs-second-review labels Jun 16, 2020
@zezha-msft zezha-msft removed this from the 10.5.0 milestone Jul 1, 2020
@zezha-msft
Copy link
Contributor

Sorry for the long delay! @cemeyer we've finally added support for in-memory login, so that keyring is not necessary and shouldn't be a blocker for other linux flavors. Here is a wiki that explain the usage further.

Could you please give it a try and let us know if you have any feedbacks? Thanks!

@zezha-msft zezha-msft closed this Nov 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-on-hold Use on PRs to indicate ones that shouldn't be reviewed right now because they're blocked on somethin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants