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

feat: add a new basic type identity #563

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

Conversation

IronCore864
Copy link
Contributor

@IronCore864 IronCore864 commented Jan 23, 2025

A new "basic" type of identity and "metrics" type of access are added for the upcoming metrics feature.

To expose the upcoming metrics feature over HTTP, we need a certain level of authentication to protect the endpoint. We decided to use HTTP basic authentication for this purpose. A new type of "basic" identity needs to be implemented for this to work, and the access level would be metrics. The basic identity looks like this:

identities:
  bob:
    access: metrics
    basic:
      password: hashed-password-with-salt

Where the password is generated by openssl passed -6. The hashed password will be stored in the state, and when the user accesses the metrics endpoint, they need to set the Authorization header accordingly. Pebble daemon will sha512 hash the password and compare it to the identity stored in the state.

For more details, see the spec here.

@IronCore864 IronCore864 requested a review from benhoyt January 23, 2025 01:51
@IronCore864 IronCore864 marked this pull request as ready for review January 23, 2025 01:51
@benhoyt
Copy link
Contributor

benhoyt commented Jan 24, 2025

Let's please add a few more details about the feature to the PR description, and link to the spec (internal, I know).

@benhoyt
Copy link
Contributor

benhoyt commented Feb 5, 2025

Just for the record, I'm pasting my little bit of research into the available sha-crypt libraries I could find. I think GehirnInc/crypt is the right choice.

https://github.com/GehirnInc/crypt
- 76 stars
- project last updated 2y ago
- file last updated 7y ago
- file last updated TODO
- originally by Jeramey Crawford
- https://github.com/GehirnInc/crypt/blob/master/sha512_crypt/sha512_crypt.go

https://github.com/tredoe/osutil
- 76 stars
- project last updated 6mo ago
- file last updated 9y ago
- originally by Jeramey Crawford, somewhat similar to GehirnInc/crypt impl
- https://github.com/tredoe/osutil/blob/master/user/crypt/sha512_crypt/sha512_crypt.go

https://github.com/ncw/pwhash
- 5 stars
- project last updated 10y ago
- file last updated 10y ago
- originally by Jeramey Crawford, somewhat similar to GehirnInc/crypt impl
https://github.com/ncw/pwhash/blob/master/sha512_crypt/sha512_crypt.go

https://github.com/go-crypt/crypt
- 16 stars
- project last updated 5 days ago
- dir last updated 2y ago (but impl is in separate xcrypt repo)
- a *lot* more API surface and complexity of implementation
- actual crypt impl is in https://github.com/go-crypt/x: 1 star, 6mo ago
- https://github.com/go-crypt/crypt/blob/master/algorithm/shacrypt/hasher.go
- https://github.com/go-crypt/x/blob/master/crypt/crypt.go

Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Thanks for this. Some minor stuff, but a couple of things worth further discussion, particularly how IdentityFromInputs will need to change.

client/identities.go Outdated Show resolved Hide resolved
internals/overlord/state/identities.go Outdated Show resolved Hide resolved
internals/overlord/state/identities.go Outdated Show resolved Hide resolved
internals/overlord/state/identities.go Outdated Show resolved Hide resolved
internals/overlord/state/identities.go Outdated Show resolved Hide resolved
internals/overlord/state/identities_test.go Outdated Show resolved Hide resolved
internals/overlord/state/identities_test.go Show resolved Hide resolved
internals/daemon/daemon.go Outdated Show resolved Hide resolved
internals/daemon/access.go Show resolved Hide resolved
internals/overlord/state/identities.go Show resolved Hide resolved
@IronCore864
Copy link
Contributor Author

In the latest commit, I have fixed most comments except for the access level part and multiple identity type part. See above comment for more information.

@benhoyt
Copy link
Contributor

benhoyt commented Feb 11, 2025

@IronCore864 Can you please merge from master now that the interface{} to any PR is merged? #568

Copy link
Contributor

@flotter flotter left a comment

Choose a reason for hiding this comment

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

This is great. Looking forward to getting this in Pebble.

"sort"
"strings"

"github.com/GehirnInc/crypt/sha512_crypt"
Copy link
Contributor

Choose a reason for hiding this comment

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

I there any reason why we use this library and not crypto/sha512 for hashing the password ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, they're different things: crypto/sha512 is the straight SHA-512 hash algorithm, but GehirnInc/crypt/sha512_crypt implements the SHA-crypt algorithm using SHA-512 hashes and a whole bunch of "rounds" and other stuff I don't understand :-) to make it good for use with passwords (slow and secure, basically).

GehirnInc/crypt/sha512_crypt uses the stdlib's crypt/sha512 in its implementation.

Local *LocalIdentity
Basic *BasicIdentity
Copy link
Contributor

Choose a reason for hiding this comment

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

This may have already been debated, but in the global scoped context of identities, would it not be a bit clearer that this refers to an HTTP authentication scheme specifically, and not a basic identity scheme in the generic sense? Or is that fact that this is simply a user / password mapping deliberately made available as a generic type identity? My first thoughts were BasicHTTP, BasicHTTPIdentity ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, in spec review with Gustavo and Harry we originally had "basic-http: ..." and other suggestions, but settled on just "basic" for brevity (not quite as explicit, I know, but it'll also potentially work with other forms of username/password auth, not just Basic HTTP. So I think we'll stick with "basic".

@IronCore864
Copy link
Contributor Author

Some manual tests:

1 Two Types of Identity

identities:
    bob:
        access: metrics
        basic:
            password: $6$F9cFSVEKyO4gB1Wh$8S1BSKsNkF.jBAixGc4W7l80OpfCNk65LZBDHBng3NAmbcHuMj4RIm7992rrJ8YA.SJ0hvm.vGk2z483am4Ym1
        local:
            user-id: 1001
$ ./pebble add-identities --from ./identity.yaml
Added 1 new identity.
$ ./pebble identities
Name  Access   Types
bob   metrics  basic,local

2 No Password for Basic Type

identities:
    nancy:
        access: metrics
        basic:
            password: 
$ ./pebble add-identities --from ./identity.yaml
error: cannot decode request body: basic identity must specify password (hashed)

3 Invalid Access Level for Basic Type

identities:
    bob:
        access: admin
        basic:
            password: $6$F9cFSVEKyO4gB1Wh$8S1BSKsNkF.jBAixGc4W7l80OpfCNk65LZBDHBng3NAmbcHuMj4RIm7992rrJ8YA.SJ0hvm.vGk2z483am4Ym1
$ ./pebble add-identities --from ./identity.yaml
error: cannot decode request body: basic identity can only have "metrics" access, got "admin"

@IronCore864 IronCore864 requested a review from benhoyt February 13, 2025 03:13
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.

4 participants