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

pass: base64 encoded username with backward compatility #288

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

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented May 28, 2023

carry and closes #267
fixes #162

The allowed characters for usernames in Unix systems typically follow these guidelines:

  • Alphanumeric characters: Both uppercase and lowercase letters (A-Z, a-z) are allowed.
  • Numeric digits: The numbers 0-9 are allowed.
  • Special characters: In most Unix systems, usernames can include the underscore character _. but also - , ..

Looking at shadow utility and the regexp used it seems to match https://github.com/shadow-maint/shadow/blob/dcc90658fd672c63e5498619e77f2d5a3d95f7d7/libmisc/chkname.c#L28-L73

But there are some cases like the credential helper where we can have other special characters to be handled. shadow also needed to allow non-standard usernames. e.g., for compatibility with Samba machine accounts: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=42874. So it seems ok to rely on base64 encoding for the username.

@codecov-commenter
Copy link

codecov-commenter commented May 28, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.46 🎉

Comparison is base (a652f8e) 54.68% compared to head (5fd7864) 55.14%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #288      +/-   ##
==========================================
+ Coverage   54.68%   55.14%   +0.46%     
==========================================
  Files           9        9              
  Lines         673      680       +7     
==========================================
+ Hits          368      375       +7     
  Misses        262      262              
  Partials       43       43              
Impacted Files Coverage Δ
pass/pass.go 69.16% <100.00%> (+1.91%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@crazy-max crazy-max force-pushed the pass-username-b64 branch 4 times, most recently from 5fd7864 to fe640fd Compare May 28, 2023 19:22
Signed-off-by: Davide Cosentino <davide.cosentino@oracle.com>
@crazy-max crazy-max marked this pull request as ready for review May 29, 2023 16:23
@crazy-max crazy-max requested a review from thaJeztah May 29, 2023 16:23
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@thaJeztah
Copy link
Member

Thanks @crazy-max - I'll try to have a look at this one.

Some things we should consider when merging some of the pending PRs;

  • We're still at v0.x.x
  • Some of the pending PRs are adding new features / drivers
  • ^^ ideally, I think we should use "minor" (e.g. v0.8.0, v0.10.0) updates for these
  • ^^ so that we "reserve" patch (v0.7.1, v0.8.1) updates in case we have bug fixes that we need, without adding new features

So, probably we should have a look at what's already merged, and what's still pending, and then decide what change should go into what "minor" or "patch" release.

I think so far, the changes that were merged since v0.7.0 are all relatively safe (mostly fixes, and the addition of the --version and --help flags); v0.7.0...83d38ea. To be on the safe side, we could tag those changes as a v0.8.0 (then we'd still have v0.7.x in case we have a problem).

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.

docker-credential-pass does not work with username containing forward-slash
4 participants