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 first cut of git-credential-fdoss #2

Closed
wants to merge 2 commits into from
Closed

Conversation

camh-
Copy link
Member

@camh- camh- commented Aug 2, 2024

Add the initial version of git-credential-fdoss. It basically works
but has some unsupported cases such as:

  • Handling "prompts"
  • Handling locked secrets (probably requires prompts to work)
  • Using a non-default keyring
  • Creating a non-existent default keyring (not even sure we we should,
    but this is something libsecret does, and hence
    git-credential-libsecret too)
  • Encrypted sessions via dbus (not "plain")
  • Non-pageable memory for holding secrets

Also missing:

  • Go unit tests
  • Expanded readme with setup instructions

Bootstrap the project with a bare-bones `main.go` with all the tooling
configured:

    hermit install go golangci-lint goreleaser make
    go mod init foxygo.at/offscreen

Use the standard foxygoat `Makefile` for Go projects (copied from
`offscreen` and slightly updated).

Configure `golangci-lint` with specific linters disabled, but all others
enabled.

Configure GitHub Actions for CI/CD.

Configure goreleaser to release Linux binaries, but no other OSes as
only Linux uses the freedesktop.org Secret Service. Maybe FreeBSD could
be added, but I know little about that.
@camh- camh- requested a review from juliaogris August 2, 2024 04:20
@camh-
Copy link
Member Author

camh- commented Aug 2, 2024

Has the do-not-merge label because it is not based on master. Once PR#1 is merged, this can be rebased and I'll then remove the label. But it could be reviewed now.

@camh- camh- changed the base branch from master to init-repo August 2, 2024 04:21
@camh- camh- force-pushed the first-cut branch 2 times, most recently from 784d93e to 1433027 Compare August 2, 2024 06:10
Add the initial version of `git-credential-fdoss`. It basically works
but has some unsupported cases such as:
* Handling "prompts"
* Handling locked secrets (probably requires prompts to work)
* Using a non-default keyring
* Creating a non-existent default keyring (not even sure we we should,
  but this is something libsecret does, and hence
  git-credential-libsecret too)
* Encrypted sessions via dbus (not "plain")
* Non-pageable memory for holding secrets

Also missing:
* Go unit tests
* Expanded readme with setup instructions
Copy link
Member

@juliaogris juliaogris left a comment

Choose a reason for hiding this comment

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

lgtm, with more questions, feel free to ignore.

Makefile Show resolved Hide resolved
dbus.go Show resolved Hide resolved
case len(locked) > 0:
fmt.Fprintln(os.Stderr, "TODO: Found locked secret. Sorry, can't unlock yet")
default:
// secret not found. return nothing
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't be an error? (same above?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope - it's fine to not have any secrets stored - you just can't return any and git will then prompt for the credentials. That's essentially how they get in there in the first place. You can also potentially have multiple credential helpers that git will call, which it will do until it has the username and password, so a credential helper should not produce an error if it does not have credentials.

return call.Err
}

func (ss *SecretService) Get(attrs map[string]string) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

A high level comment here what be helpful, especially with a sample of attrs.


// Run cleans up after a command. It is called after any commands are run.
func (cmd *CLI) Run(ss *SecretService) error {
// This close is not strictly necessary as the session is close automatically
Copy link
Member

Choose a reason for hiding this comment

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

is closed

case "oauth_refresh_token":
gc.OauthRefreshToken = v
default:
// Ignore unknown fields for forward compatibiilty
Copy link
Member

Choose a reason for hiding this comment

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

I like this clear call out to why we ignore unknown fields, same above.

@camh- camh- force-pushed the init-repo branch 4 times, most recently from bc4d4e1 to 281f9c7 Compare August 7, 2024 07:51
@camh- camh- deleted the branch init-repo August 7, 2024 07:55
@camh- camh- closed this Aug 7, 2024
camh- added a commit that referenced this pull request Sep 6, 2024
When retrieving secrets that match the attributes we have, ignore any
attributes that we would not have added ourselves. It is conceivable
that another service or the user adds additional attributes to a secret
for their own purposes. We only care about exactly matching the
attributes we would have added ourselves, so do just that.

While we're here, tidy up the `SecretService.Store()` method and upgrade
the `gh` command managed by hermit.

This merges the following commits:
* Compare only attributes we may have added
* Tidy up SecretService.Store
* hermit: Upgrade gh

     bin/{.gh-2.54.0.pkg => .gh-2.55.0.pkg} |  0
     bin/gh                                 |  2 +-
     dbus.go                                | 40 ++++++++++++++------------
     main.go                                |  5 ++++
     4 files changed, 27 insertions(+), 20 deletions(-)

Pull-request: #10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants