-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[sec_scan][5] add secrets backend service #43543
Conversation
b06e835
to
59ef61d
Compare
8f7e716
to
14e2b8b
Compare
This PR implements the backend service to support storing `authorized_keys` and `private_keys` into Teleport backend. Part of gravitational/access-graph#637 Signed-off-by: Tiago Silva <[email protected]>
14e2b8b
to
9380571
Compare
Friendly ping @smallinsky @rudream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have left a few suggestions but I have limited understending how/when keys will be upserted.
Itself the backend storage functionality LGTM but I have left a few concerns about naming.
Expires: timestamppb.New( | ||
time.Now().Add(defaultKeyTTL), | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In current design it will be not way to mock the expiration time.
Does it make sense to inject the header and allow to set the name and the expiration time from outside ?
func NewAuthorizedKey(metadata header.Metadata, spec *accessgraphv1pb.AuthorizedKeySpec) (*accessgraphv1pb.AuthorizedKey, error)
and generate the name value is CheckAndSetDefault
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't a big issue because you can always reset it after.
if k.Spec.PublicKeyFingerprint == "" && k.Spec.PublicKeyMode != accessgraphv1pb.PublicKeyMode_PUBLIC_KEY_MODE_PROTECTED { | ||
return trace.BadParameter("PublicKeyFingerprint is unset") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit confusing the there PrivateKeySpec
has PublicKeyFingerprint
and itself the references to Public Key and the any uniq members related to private key.
Does it make sense to rename it to something like: SSHKeyDetails
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the PrivateKeySpec holds the public key fingerprint.
We do read the private key, extract the public key from it and compute the SHA256 fingerprint before sending it to teleport.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the private key needs to be read at first place. Does it make sense to read the public key and get the signature without read to read sensitive private key ?
} | ||
|
||
// ListPrivateKeysForDevice lists all private keys for a given deviceID. | ||
func (k *AccessGraphSecretsService) ListPrivateKeysForDevice(ctx context.Context, deviceID string, pageSize int, pageToken string) ([]*accessgraphsecretspb.PrivateKey, string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first glance this look scary that we are listing PrivateKeys. Can we just rework this to ListSSHKeysDetails
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are not private keys. We call them private keys because they represent a private key in the host machine but they actually just store the fingerprint of the public key derived from a private key.
There is no private key material leaving the user's laptop so this is 100% safe.
besides that, there won't be a public api to export these findings to the outside.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Marek. If this API doesn't list private keys, I'd not call it "ListPrivateKeys".
|
||
// UpsertPrivateKey upserts a new private key. | ||
func (k *AccessGraphSecretsService) UpsertPrivateKey(ctx context.Context, in *accessgraphsecretspb.PrivateKey) (*accessgraphsecretspb.PrivateKey, error) { | ||
svc := k.privateKeysSvc.WithPrefix(in.Spec.DeviceId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonder why the deviceId prefix is needed the name consist with user + deviceID and fingerprint hash.
Does the backend will also have ability to list all keys from particular machine. If not the DeviceID prefix can be removed from backend item path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since each device reports all the new findings, we need to list the last reported data for a certain device to reconcile. everything happens server side!
} | ||
|
||
// ListPrivateKeysForDevice lists all private keys for a given deviceID. | ||
func (k *AccessGraphSecretsService) ListPrivateKeysForDevice(ctx context.Context, deviceID string, pageSize int, pageToken string) ([]*accessgraphsecretspb.PrivateKey, string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Marek. If this API doesn't list private keys, I'd not call it "ListPrivateKeys".
* [sec_scan][5] add secrets backend service This PR implements the backend service to support storing `authorized_keys` and `private_keys` into Teleport backend. Part of gravitational/access-graph#637 Signed-off-by: Tiago Silva <[email protected]> * handle feedback * handle nits --------- Signed-off-by: Tiago Silva <[email protected]>
* [sec_scan][5] add secrets backend service This PR implements the backend service to support storing `authorized_keys` and `private_keys` into Teleport backend. Part of gravitational/access-graph#637 Signed-off-by: Tiago Silva <[email protected]> * handle feedback * handle nits --------- Signed-off-by: Tiago Silva <[email protected]>
* Add the device assertion protos (#43804) * Add the device assertion protos * Update generated protos * Add a client-side API to assert devices (#43890) * Add a client-side API to assert devices * Add a godoc to authnStreamAdapter * Define server-side device assertion interfaces (#44036) * Define server-side device assertion interfaces * Update proto comments * Update generated protos * [sec_scan][1] Add `teleport.access_graph.v1.SecretsScannerService` (#43462) This PR introduces the `teleport.access_graph.v1.SecretsScannerService`that will be used by Teleport SSH nodes to report `authorized_keys` and user's laptops to report secrets found on them. The `ReportAuthorizedKeys` uses node's TLS certs signed by HostCA for authentication while `ReportSecrets` leverages the device trust credentials (requires that the device is enrolled) to report secrets without requiring valid user credentials. handle Alan's feedback * [sec_scan][2] expose `ssh_scan_enabled` in `AccessGraphConfig` response (#43467) This PR exposes the configuration for nodes to be aware that they should report SSH Authorized keys to Teleport. Part of gravitational/access-graph#637 Signed-off-by: Tiago Silva <[email protected]> * [sec_scan][3] add `PrivateKey`, `AuthorizedKey` and `Device` to Access Graph resources (#43468) This PR extends the Access Graph resources to be able include the newly added `teleport.access_graph.v1.PrivateKey`, `teleport.access_graph.v1.AuthorizedKey` and existing device trust information `teleport.devicetrust.v1.Device`. Part of gravitational/access-graph#637 Signed-off-by: Tiago Silva <[email protected]> * fix: fix `nextKey` values when using multiple prefixes (#43486) This PR makes `generic.Service` correctly implementing `List*` functions when multiple key prefixes are defined Signed-off-by: Tiago Silva <[email protected]> * [sec_scan][5] add secrets backend service (#43543) * [sec_scan][5] add secrets backend service This PR implements the backend service to support storing `authorized_keys` and `private_keys` into Teleport backend. Part of gravitational/access-graph#637 Signed-off-by: Tiago Silva <[email protected]> * handle feedback * handle nits --------- Signed-off-by: Tiago Silva <[email protected]> * [sec_scan][6] add device events (#43905) This PR adds the ability to watch for events for `*devicepb.Device` objects. Backend storage representation of `devicepb.Device` is achieved using an internal representation that lives in `e/lib/devicetrust/storage` and whose logic is internal to the package. To be able to expose the unmarshal logic necessary for events to work, this PR exposes a registration hook that `e/lib/devicetrust/storage` function must call during initialization to register the unmarshal function. Part of gravitational/access-graph#637 Signed-off-by: Tiago Silva <[email protected]> * [sec_scan][7] add authorizedKeys and privateKeys events support (#43906) This PR introduces the ability to watch for events related to `accessgraphsecretsv1pb.AuthorizedKey` and `accessgraphsecretsv1pb.PrivateKey` objects. This PR is part of gravitational/access-graph#637. Signed-off-by: Tiago Silva <[email protected]> * [sec_scan][9] add `access_graph_settings` protobuf (#44010) This PR adds the `clusterconfigpbv1.AccessGraphSettings` resource that will be used to control the secrets scanning definition of Teleport. This resource will be a singleton and the only goal is to carry some settings related to access graph because on the cloud, users don't have access to fileconf. This PR is part of gravitational/access-graph#637. Signed-off-by: Tiago Silva <[email protected]> * [sec_scan][10] add `AccessGraphSettingsUpdate` audit event (#44011) This PR adds the boilerplate code and proto definition for `AccessGraphSettingsUpdate` audit event. This PR is part of gravitational/access-graph#637. Signed-off-by: Tiago Silva <[email protected]> * [sec_scan][11] add `AccessGraphSettings` backend service (#44014) This PR adds the backend service to be able to create, update and retrieve access graph configurations from Teleport backend. This PR is part of gravitational/access-graph#637. * [sec_scan][12] add cache and events support for `AccessGraphSettings` (#44016) * [sec_scan][12] add cache and events support for `AccessGraphSettings` This PR adds the cache and events support for the new resource `AccessGraphSettings`. This PR is part of gravitational/access-graph#637. Signed-off-by: Tiago Silva <[email protected]> * add tests --------- Signed-off-by: Tiago Silva <[email protected]> * [sec_scan][13] add `AccessGraphSettings` gRPC implementation (#44021) This PR introduces the gRPC implementation for the CRUD operations related to `AccessGraphSettings`. This PR is part of gravitational/access-graph#637. Signed-off-by: Tiago Silva <[email protected]> * [sec_scan][14] create `AccessGraphSettings` on first auth init (#44032) * [sec_scan][14] create `AccessGraphSettings` on first auth init This PR adds a init script that sets `AccessGraphSettings` into Teleport backend when auth first inits and there is no `AccessGraphSettings`. This PR is part of gravitational/access-graph#637. Signed-off-by: Tiago Silva <[email protected]> * remove iterations --------- Signed-off-by: Tiago Silva <[email protected]> * [sec_scan][15] add support for edits to `AccessGraphSettings` via `tctl` (#44055) This PR allows any cluster admin to edit `access_graph_settings` objects via `tctl`. This PR is part of gravitational/access-graph#637. Signed-off-by: Tiago Silva <[email protected]> * [sec_scan][16] add methods to store/retrieve device assertion functions (#44081) This PR adds methods to store/retrieve functions defined by different teleport services. This PR is part of gravitational/access-graph#637. Signed-off-by: Tiago Silva <[email protected]> * [sec_scan][17] add `AssertDevice` to `FakeDeviceService` (#44159) * [sec_scan][17] add `AssertDevice` to `FakeDeviceService` This PR introduces a `AssertDevice` logic into `FakeDeviceService` to authenticate devices during unit tests using device trust credentials. This PR is part of gravitational/access-graph#637. Signed-off-by: Tiago Silva <[email protected]> * simplify assert tests * Update lib/devicetrust/assert/assert_test.go Co-authored-by: Alan Parra <[email protected]> --------- Signed-off-by: Tiago Silva <[email protected]> Co-authored-by: Alan Parra <[email protected]> * [sec_scan][20] add `ReportSecrets` forwarder to proxy's gRPC insecure server (#44324) * [sec_scan][20] add `ReportSecrets` forwarder to proxy's gRPC insecure server This PR implements a `ReportSecrets` forwarder from Proxy server to Auth server. The goal is to allow clients to hit the proxy insecure gRPC server (credentialless) and proxy will forward requests to the AuthServer on behalf of the client. This is required because the client doesn't have valid credentials and it wasn't possible for it to reach auth server via reversetunnel when the cluster uses `separate` mode. This PR is part of gravitational/access-graph#637. Signed-off-by: Tiago Silva <[email protected]> * add comments * move dial to lib/client/proxy/insecure --------- Signed-off-by: Tiago Silva <[email protected]> * [sec_scan][19] add `tsh scan keys` implementation (#44220) * [sec_scan][19] add `tsh scan keys` implementation This PR introduces the required code to transverse a directory(es), finding all the SSH private keys and report them back to the cluster using the device security enclave as authentication mechanism. This PR is part of gravitational/access-graph#637. Signed-off-by: Tiago Silva <[email protected]> * handle code review * fix message * handle code review * fork ssh private keys * add skip dirs support * handle code review --------- Signed-off-by: Tiago Silva <[email protected]> * [sec_scan][22] add authorized keys reporter (#44523) * [sec_scan][22] add authorized keys reporter This PR introduces a SSH authorized keys reporter that monitors `/etc/passwd` file and all users' authorized_keys files and reports the findings back to teleport. Part of gravitational/access-graph#637 Signed-off-by: Tiago Silva <[email protected]> * handle comments * handle comments --------- Signed-off-by: Tiago Silva <[email protected]> * [sec_scan][24] extract AuthorizedKey's comment and type (#44643) This PR adds ability to extract the comment and key type from AuthorizedKeys files. Signed-off-by: Tiago Silva <[email protected]> * fix api module * [sec_scan][27] add support for LDAP users and macOS (#45109) * [sec_scan][27] add support for LDAP users and macOS This PR extends support for authorized keys report for users managed by LDAP system and macOS targets. It leverages `getpwent` to read the system database files and retrieve the user properties. It doesn't use the `getpwent_r` because it's not available in macOS and because it's not (yet) standerdized > PLEASE NOTE: the `getpwent_r' function is not (yet) standardized. > The interface may change in later versions of this library. But > the interface is designed following the principals used for the > other reentrant functions so the chances are good this is what the > POSIX people would choose. Part of gravitational/access-graph#637 * handle comments * handle comments 2 * add comment --------- Signed-off-by: Tiago Silva <[email protected]> Co-authored-by: Alan Parra <[email protected]>
* Add the device assertion protos (#43804) * Add the device assertion protos * Update generated protos * Add a client-side API to assert devices (#43890) * Add a client-side API to assert devices * Add a godoc to authnStreamAdapter * Define server-side device assertion interfaces (#44036) * Define server-side device assertion interfaces * Update proto comments * Update generated protos * [sec_scan][1] Add `teleport.access_graph.v1.SecretsScannerService` (#43462) This PR introduces the `teleport.access_graph.v1.SecretsScannerService`that will be used by Teleport SSH nodes to report `authorized_keys` and user's laptops to report secrets found on them. The `ReportAuthorizedKeys` uses node's TLS certs signed by HostCA for authentication while `ReportSecrets` leverages the device trust credentials (requires that the device is enrolled) to report secrets without requiring valid user credentials. handle Alan's feedback * [sec_scan][2] expose `ssh_scan_enabled` in `AccessGraphConfig` response (#43467) This PR exposes the configuration for nodes to be aware that they should report SSH Authorized keys to Teleport. Part of gravitational/access-graph#637 Signed-off-by: Tiago Silva <[email protected]> * [sec_scan][3] add `PrivateKey`, `AuthorizedKey` and `Device` to Access Graph resources (#43468) This PR extends the Access Graph resources to be able include the newly added `teleport.access_graph.v1.PrivateKey`, `teleport.access_graph.v1.AuthorizedKey` and existing device trust information `teleport.devicetrust.v1.Device`. Part of gravitational/access-graph#637 Signed-off-by: Tiago Silva <[email protected]> * fix: fix `nextKey` values when using multiple prefixes (#43486) This PR makes `generic.Service` correctly implementing `List*` functions when multiple key prefixes are defined Signed-off-by: Tiago Silva <[email protected]> * [sec_scan][5] add secrets backend service (#43543) * [sec_scan][5] add secrets backend service This PR implements the backend service to support storing `authorized_keys` and `private_keys` into Teleport backend. Part of gravitational/access-graph#637 Signed-off-by: Tiago Silva <[email protected]> * handle feedback * handle nits --------- Signed-off-by: Tiago Silva <[email protected]> * [sec_scan][6] add device events (#43905) This PR adds the ability to watch for events for `*devicepb.Device` objects. Backend storage representation of `devicepb.Device` is achieved using an internal representation that lives in `e/lib/devicetrust/storage` and whose logic is internal to the package. To be able to expose the unmarshal logic necessary for events to work, this PR exposes a registration hook that `e/lib/devicetrust/storage` function must call during initialization to register the unmarshal function. Part of gravitational/access-graph#637 Signed-off-by: Tiago Silva <[email protected]> * [sec_scan][7] add authorizedKeys and privateKeys events support (#43906) This PR introduces the ability to watch for events related to `accessgraphsecretsv1pb.AuthorizedKey` and `accessgraphsecretsv1pb.PrivateKey` objects. This PR is part of gravitational/access-graph#637. Signed-off-by: Tiago Silva <[email protected]> * [sec_scan][9] add `access_graph_settings` protobuf (#44010) This PR adds the `clusterconfigpbv1.AccessGraphSettings` resource that will be used to control the secrets scanning definition of Teleport. This resource will be a singleton and the only goal is to carry some settings related to access graph because on the cloud, users don't have access to fileconf. This PR is part of gravitational/access-graph#637. Signed-off-by: Tiago Silva <[email protected]> * [sec_scan][10] add `AccessGraphSettingsUpdate` audit event (#44011) This PR adds the boilerplate code and proto definition for `AccessGraphSettingsUpdate` audit event. This PR is part of gravitational/access-graph#637. Signed-off-by: Tiago Silva <[email protected]> * [sec_scan][11] add `AccessGraphSettings` backend service (#44014) This PR adds the backend service to be able to create, update and retrieve access graph configurations from Teleport backend. This PR is part of gravitational/access-graph#637. * [sec_scan][12] add cache and events support for `AccessGraphSettings` (#44016) * [sec_scan][12] add cache and events support for `AccessGraphSettings` This PR adds the cache and events support for the new resource `AccessGraphSettings`. This PR is part of gravitational/access-graph#637. Signed-off-by: Tiago Silva <[email protected]> * add tests --------- Signed-off-by: Tiago Silva <[email protected]> * [sec_scan][13] add `AccessGraphSettings` gRPC implementation (#44021) This PR introduces the gRPC implementation for the CRUD operations related to `AccessGraphSettings`. This PR is part of gravitational/access-graph#637. Signed-off-by: Tiago Silva <[email protected]> * [sec_scan][14] create `AccessGraphSettings` on first auth init (#44032) * [sec_scan][14] create `AccessGraphSettings` on first auth init This PR adds a init script that sets `AccessGraphSettings` into Teleport backend when auth first inits and there is no `AccessGraphSettings`. This PR is part of gravitational/access-graph#637. Signed-off-by: Tiago Silva <[email protected]> * remove iterations --------- Signed-off-by: Tiago Silva <[email protected]> * [sec_scan][15] add support for edits to `AccessGraphSettings` via `tctl` (#44055) This PR allows any cluster admin to edit `access_graph_settings` objects via `tctl`. This PR is part of gravitational/access-graph#637. Signed-off-by: Tiago Silva <[email protected]> * [sec_scan][16] add methods to store/retrieve device assertion functions (#44081) This PR adds methods to store/retrieve functions defined by different teleport services. This PR is part of gravitational/access-graph#637. Signed-off-by: Tiago Silva <[email protected]> * [sec_scan][17] add `AssertDevice` to `FakeDeviceService` (#44159) * [sec_scan][17] add `AssertDevice` to `FakeDeviceService` This PR introduces a `AssertDevice` logic into `FakeDeviceService` to authenticate devices during unit tests using device trust credentials. This PR is part of gravitational/access-graph#637. Signed-off-by: Tiago Silva <[email protected]> * simplify assert tests * Update lib/devicetrust/assert/assert_test.go Co-authored-by: Alan Parra <[email protected]> --------- Signed-off-by: Tiago Silva <[email protected]> Co-authored-by: Alan Parra <[email protected]> * [sec_scan][20] add `ReportSecrets` forwarder to proxy's gRPC insecure server (#44324) * [sec_scan][20] add `ReportSecrets` forwarder to proxy's gRPC insecure server This PR implements a `ReportSecrets` forwarder from Proxy server to Auth server. The goal is to allow clients to hit the proxy insecure gRPC server (credentialless) and proxy will forward requests to the AuthServer on behalf of the client. This is required because the client doesn't have valid credentials and it wasn't possible for it to reach auth server via reversetunnel when the cluster uses `separate` mode. This PR is part of gravitational/access-graph#637. Signed-off-by: Tiago Silva <[email protected]> * add comments * move dial to lib/client/proxy/insecure --------- Signed-off-by: Tiago Silva <[email protected]> * [sec_scan][19] add `tsh scan keys` implementation (#44220) * [sec_scan][19] add `tsh scan keys` implementation This PR introduces the required code to transverse a directory(es), finding all the SSH private keys and report them back to the cluster using the device security enclave as authentication mechanism. This PR is part of gravitational/access-graph#637. Signed-off-by: Tiago Silva <[email protected]> * handle code review * fix message * handle code review * fork ssh private keys * add skip dirs support * handle code review --------- Signed-off-by: Tiago Silva <[email protected]> * [sec_scan][22] add authorized keys reporter (#44523) * [sec_scan][22] add authorized keys reporter This PR introduces a SSH authorized keys reporter that monitors `/etc/passwd` file and all users' authorized_keys files and reports the findings back to teleport. Part of gravitational/access-graph#637 Signed-off-by: Tiago Silva <[email protected]> * handle comments * handle comments --------- Signed-off-by: Tiago Silva <[email protected]> * [sec_scan][24] extract AuthorizedKey's comment and type (#44643) This PR adds ability to extract the comment and key type from AuthorizedKeys files. Signed-off-by: Tiago Silva <[email protected]> * update gomod * [sec_scan][27] add support for LDAP users and macOS (#45109) * [sec_scan][27] add support for LDAP users and macOS This PR extends support for authorized keys report for users managed by LDAP system and macOS targets. It leverages `getpwent` to read the system database files and retrieve the user properties. It doesn't use the `getpwent_r` because it's not available in macOS and because it's not (yet) standerdized > PLEASE NOTE: the `getpwent_r' function is not (yet) standardized. > The interface may change in later versions of this library. But > the interface is designed following the principals used for the > other reentrant functions so the chances are good this is what the > POSIX people would choose. Part of gravitational/access-graph#637 * handle comments * handle comments 2 * add comment --------- Signed-off-by: Tiago Silva <[email protected]> Co-authored-by: Alan Parra <[email protected]>
This PR implements the backend service to support storing
authorized_keys
andprivate_keys
into Teleport backend.Part of https://github.com/gravitational/access-graph/issues/637