[sec_scan][19] add tsh scan keys implementation
#44220
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tigrato
force-pushed
the
tigrato/ssh-keys-impl11
branch
from
7602ff2 to
f19b92a
Compare
tigrato
added
backport/branch/v14
no-changelog
github-actions
bot
added
size/md
tsh
codingllama
reviewed
Jul 17, 2024
tigrato
force-pushed
the
tigrato/ssh-keys-impl13
branch
from
406f2ba to
d59e543
Compare
tigrato
force-pushed
the
tigrato/ssh-keys-impl11
branch
from
f19b92a to
4644101
Compare
justinas
approved these changes
Jul 18, 2024
tigrato
force-pushed
the
tigrato/ssh-keys-impl13
branch
from
fd27c60 to
a2c56e0
Compare
codingllama
reviewed
Jul 18, 2024
lib/secretsscanner/scan/scan.go
Outdated
| defer file.Close() | ||
|
|
||
| // read the first 150 bytes of the file to check if it's an OpenSSH private key. | ||
| var buf [150]byte |
There was a problem hiding this comment.
As an aside, why did you pick 150? Are we guaranteed no key will ever be smaller than that?
There was a problem hiding this comment.
We expect the key to be pem encoded and just the BEGIN + END are arround 100 bytes.
There is no key in pem encoded format that is bellow 150 bytes but I reduced it to the maximum BEGIN size which is 40
lib/secretsscanner/scan/scan_test.go
Outdated
| err := os.Mkdir(filepath.Join(dir, key.Name), os.ModePerm) | ||
| require.NoError(t, err) | ||
|
|
||
| filePath := filepath.Join(dir, key.Name, key.Name) |
There was a problem hiding this comment.
to ensure the walk function iterates through all directories and subdirectories
There was a problem hiding this comment.
Please add a brief comment so it's clearer.
lib/secretsscanner/scan/scan_test.go
Outdated
| "github.com/google/uuid" | ||
| "github.com/stretchr/testify/require" | ||
| "golang.org/x/crypto/ssh" | ||
| cryptosshtestdata "golang.org/x/crypto/ssh/testdata" |
There was a problem hiding this comment.
Is it wise to import test data from a third party package?
This seems like the kind of package that could easily change or disappear during a version update, leaving us broken.
It also seems like our test scenarios could silently shift without us realizing it.
There was a problem hiding this comment.
I attempted to avoid it since Panther would flag a secret leak in this PR.
Anyway, I forked the ssh's test data private keys, retained the credits, and we can proceed with it.
lib/secretsscanner/scan/scan_test.go
Outdated
| return expectedKeys | ||
| } | ||
|
|
||
| func writeEncryptedKeyWithoutPubFile(t *testing.T, dir string) []*accessgraphsecretsv1pb.PrivateKey { |
There was a problem hiding this comment.
Doesn't this naturally happen on writeEncryptedKeys, by virtual of not all keys having "IncludesPublicKey: true"?
There was a problem hiding this comment.
IncludesPublicKey indicates that the passphrase-protected private key contains the un-encrypted public key in its headers. This allows for the extraction of the actual public key without needing to know the private key's passphrase.
In writeEncryptedKeys, I used the passphrase to decrypt the RSA private key and extract the public key to create a .pub file. Here, I am focusing on the scenario where the public key does not exist at all.
There was a problem hiding this comment.
What I meant is that this is already covered by test keys that do not have "IncludesPublicKey=true", right?
tigrato
force-pushed
the
tigrato/ssh-keys-impl13
branch
from
a2c56e0 to
e84919f
Compare
codingllama
reviewed
Jul 18, 2024
lib/secretsscanner/scan/scan.go
Outdated
| func (s *Scanner) findPrivateKeys(ctx context.Context, root, deviceID string) { | ||
| logger := s.log.With("dir", root) | ||
|
|
||
| err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error { |
There was a problem hiding this comment.
Given the size of the Walk and the amount of I/O we'll cause, have you considered:
- Scanning hotspots first, like /Users/{user}/.ssh, and reporting those keys right away
- Walking with multiple goroutines
- Conversely to 2), walking cold paths at a much lower priority to not hog the disk
- Avoiding certain paths during walks (~/Library, node_modules, maybe even Git repositories?, etc)
- Possibly reducing the 150 bytes "pre-read" to the bare minimum to identify a PEM file?
Also, how long does a walk take in your machine?
There was a problem hiding this comment.
On my machine, it takes approximately 4 minutes to scan 1,842,916 items in the /Users/ directory, which includes the entire go mod cache storage.
The goal is to run these scripts during off-hours, such as nights or weekends. While we can parallelize the loop, I'm unsure if it's worth the effort given the relatively quick scan time.
There was a problem hiding this comment.
That's... not as bad as I thought. Considering the size of ~/Library, .git directories, node_modules, etc, full of small files, each causing at least 150 (or 40?) bytes of I/O.
I still think it might be worth it to scan "hot spots" first, as I expect we'll hit some worst case scenarios eventually (slow machines with LOTS of files), but that could be a follow up (if you think it's worth doing).
There was a problem hiding this comment.
It's worth but not sure if it's worth now. I will revisit it once everything else lands
There was a problem hiding this comment.
Oh yes, these are all suggestions for later. Feel free to resolve.
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]>
tigrato
force-pushed
the
tigrato/ssh-keys-impl11
branch
from
438ae9e to
35e68a1
Compare
![orca-security-us[bot]](https://avatars.githubusercontent.com/in/276250?s=60&v=4){kind=small}
There was a problem hiding this comment.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
 Passed |
Infrastructure as Code |  0  0  0  0 |
View in Orca |
 Failed |
Secrets | 19 0 0 0 |
View in Orca |
| Passed |
Vulnerabilities | 0 0 0 0 |
View in Orca |
🔑 The following Secrets have been detected in your pull request across all commits
| NAME | FILE | LINE NUM | COMMIT | ||
|---|---|---|---|---|---|
|
Private Key | ...testdata/ssh_keys.go | 207 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 162 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 29 | 5a8c74f | View in code |
|
Private Key | ...data/invalid_keys.go | 44 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 280 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 150 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 131 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 115 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 107 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 140 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 42 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 35 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 23 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 265 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 168 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 243 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 78 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 50 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 10 | 5a8c74f | View in code |
|
@tigrato - this PR will require admin approval to merge due to its size. Consider breaking it up into a series smaller changes. |
codingllama
reviewed
Jul 19, 2024
| require.NoError(t, err) | ||
|
|
||
| err = os.WriteFile(filepath.Join(dir, "invalid-key-invalid-header"), []byte( | ||
| `abcefg-----BEGIN OPENSSH PRIVATE KEY-----\n |
There was a problem hiding this comment.
Since we have confirmed that this errors, should we change our bytes.Contains check to bytes.HasPrefix? (Scanner.readFileIfSSHPrivateKey)
There was a problem hiding this comment.
I wanted to avoid that because pem decode supports \n-----BEGIN OPENSSH PRIVATE KEY-----
There was a problem hiding this comment.
bytes.HasPrefix(bytes.TrimLeft(b, "\n "), "-----BEGIN OPENSSH PRIVATE KEY----")
or, if you'd rather not do that, leave a comment explaining why we do a Contains check instead of HasPrefix.
I would also add the \n-----BEGIN OPENSSH PRIVATE KEY----- to the scenarios.
Comment on lines
+45
to
+46
| // disable TLS routing check for tests | ||
| t.Setenv(defaults.TLSRoutingConnUpgradeEnvVar, "false") |
There was a problem hiding this comment.
Why do we need to set this? Is it for "secretsscannerclient.NewSecretsScannerServiceClient"?
There was a problem hiding this comment.
Yes, it includes a check for TLS routing, which we need to override because we don't run a full proxy API.
There was a problem hiding this comment.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
| Passed |
Infrastructure as Code | 0 0 0 0 |
View in Orca |
| Failed |
Secrets | 19 0 0 0 |
View in Orca |
| Passed |
Vulnerabilities | 0 0 0 0 |
View in Orca |
🔑 The following Secrets have been detected in your pull request across all commits
| NAME | FILE | LINE NUM | COMMIT | ||
|---|---|---|---|---|---|
|
Private Key | ...testdata/ssh_keys.go | 78 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 35 | 5a8c74f | View in code |
|
Private Key | ...data/invalid_keys.go | 44 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 168 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 162 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 115 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 265 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 207 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 42 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 280 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 50 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 29 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 23 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 243 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 131 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 140 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 107 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 10 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 150 | 5a8c74f | View in code |
codingllama
approved these changes
Jul 19, 2024
tigrato
force-pushed
the
tigrato/ssh-keys-impl11
branch
from
4da0e1b to
7138ae2
Compare
There was a problem hiding this comment.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
| Passed |
Infrastructure as Code | 0 0 0 0 |
View in Orca |
| Failed |
Secrets | 19 0 0 0 |
View in Orca |
| Passed |
Vulnerabilities | 0 0 0 0 |
View in Orca |
🔑 The following Secrets have been detected in your pull request across all commits
| NAME | FILE | LINE NUM | COMMIT | ||
|---|---|---|---|---|---|
|
Private Key | ...testdata/ssh_keys.go | 162 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 115 | 5a8c74f | View in code |
|
Private Key | ...data/invalid_keys.go | 44 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 207 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 42 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 23 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 265 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 168 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 150 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 140 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 107 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 78 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 29 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 10 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 280 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 243 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 131 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 50 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 35 | 5a8c74f | View in code |
tigrato
force-pushed
the
tigrato/ssh-keys-impl11
branch
from
7138ae2 to
def568e
Compare
There was a problem hiding this comment.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
| Passed |
Infrastructure as Code | 0 0 0 0 |
View in Orca |
| Failed |
Secrets | 19 0 0 0 |
View in Orca |
| Passed |
Vulnerabilities | 0 0 0 0 |
View in Orca |
🔑 The following Secrets have been detected in your pull request across all commits
| NAME | FILE | LINE NUM | COMMIT | ||
|---|---|---|---|---|---|
|
Private Key | ...testdata/ssh_keys.go | 131 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 50 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 35 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 29 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 140 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 168 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 107 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 42 | 5a8c74f | View in code |
|
Private Key | ...data/invalid_keys.go | 44 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 280 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 265 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 207 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 162 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 78 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 23 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 243 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 150 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 115 | 5a8c74f | View in code |
|
Private Key | ...testdata/ssh_keys.go | 10 | 5a8c74f | View in code |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
tigrato
added a commit
that referenced
this pull request
Jul 30, 2024
* [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]>
tigrato
added a commit
that referenced
this pull request
Jul 30, 2024
* [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]>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Aug 13, 2024
* 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]>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Aug 13, 2024
* 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]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 https://github.com/gravitational/access-graph/issues/637.