gitops, basic apis, and table for android certificate templates - #35788
Conversation
| UNIQUE KEY idx_cert_team_name (team_id, name), | ||
| FOREIGN KEY (team_id) REFERENCES teams (id), | ||
| FOREIGN KEY (certificate_authority_id) REFERENCES certificate_authorities (id) | ||
| ) DEFAULT CHARSET = utf8mb4 COLLATE = utf8mb4_unicode_ci; |
There was a problem hiding this comment.
@getvictor @marko-lisica I wasn't at the spec meetings and there isn't a ton of detail on the tickets, so trying to understand if I am correctly interpreting what is needed for this new certificate flow.
I see a bunch of other tables in our db such as scep_certificates and wstep_certificates. I assume these certificates are different. Is certificates the correct name for this table? Or should it be something like custom_certificates?
Is the FK to the certificate_authorities table is correct?
The unique constraint on team_id and name?
Do I need a platform column on here? (android, macos, windows... etc)
There was a problem hiding this comment.
I would say the table shoudl be called like certificate_templates since we are not storing actual certificates but information how to get them. We could also call it certificate_profiles if that's closer to Fleet vocabulary. I wouldn't worry about platform column until we need one.
There was a problem hiding this comment.
👍 sounds good thank you for the guidance.
There was a problem hiding this comment.
I think certificate_templates makes sense. It's similar to profile, except that it's not executed on the host. It just provides information to our agent app to build CSR. I think other tools call this certificate template.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #35788 +/- ##
==========================================
- Coverage 65.99% 65.96% -0.04%
==========================================
Files 2125 2130 +5
Lines 180946 181374 +428
Branches 7521 7419 -102
==========================================
+ Hits 119419 119645 +226
- Misses 50608 50777 +169
- Partials 10919 10952 +33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
GitHub Actions
Failure
| func (svc *Service) ApplyCertificateTemplateSpecs(ctx context.Context, specs []*fleet.CertificateRequestSpec) error { | ||
| // TODO: What is the right authorization here? | ||
| // svc.authz.Authorize(ctx, &fleet.Certificate{TeamID: tmID}, fleet.ActionWrite) ? | ||
| if err := svc.authz.Authorize(ctx, &fleet.Team{}, fleet.ActionWrite); err != nil { |
There was a problem hiding this comment.
@marko-lisica @getvictor what do permissions look like around this?
There was a problem hiding this comment.
@ksykulev Permissions changes: Already covered by "Edit OS settings" rows in permissions.
ksykulev
changed the title
|
|
||
| type deleteCertificateTemplateSpecsRequest struct { | ||
| IDs []uint `json:"ids"` | ||
| // TeamID uint `json:"team_id"` ?? |
There was a problem hiding this comment.
I'm wondering if I should pass a team into the delete request like I do to the batch apply. That would make this endpoint more safe by disallowing users to delete certificates on teams outside of their own.
| s.DoJSON("GET", fmt.Sprintf("/api/v1/fleetd/certificates/%d", certID), nil, http.StatusBadRequest, &getCertResp) | ||
|
|
||
| // Get certificate with node_key (should return replaced variables) | ||
| s.DoJSON("GET", fmt.Sprintf("/api/v1/fleetd/certificates/%d?node_key=%s", certID, *host.NodeKey), nil, http.StatusOK, &getCertResp) |
There was a problem hiding this comment.
Were we going to put node_key in the Authorization header? Putting it as a URL param exposes it in logs/OTEL/etc.
cc: @mostlikelee @sgress454
Can be fixed in a subsequent PR.
sgress454
left a comment
sgress454
left a comment
There was a problem hiding this comment.
Looking good, mainly nits! Only functional change is, we can't bail out early in GitOps if there's no certs listed, because we need to be able to delete all the certs by removing them from GitOps.
| type getDeviceCertificateTemplateRequest struct { | ||
| ID uint `url:"id"` | ||
| NodeKey string `query:"node_key"` | ||
| } | ||
|
|
||
| func (r *getDeviceCertificateTemplateRequest) hostNodeKey() string { | ||
| return r.NodeKey | ||
| } |
There was a problem hiding this comment.
I don't have much of an opinion here, but the docs have us checking an Authorization: Node key <node_key> header and @getvictor indicated that was best practice.
| if androidSettings.Certificates.Valid { | ||
| for i, cert := range androidSettings.Certificates.Value { | ||
| if cert.Name == "" { | ||
| multiError = multierror.Append(multiError, fmt.Errorf("android_settings.certificates[%d]: name is required", i)) | ||
| } | ||
| if cert.CertificateAuthorityName == "" { | ||
| multiError = multierror.Append(multiError, fmt.Errorf("android_settings.certificates[%d]: certificate_authority_name is required", i)) | ||
| } | ||
| if cert.SubjectName == "" { | ||
| multiError = multierror.Append(multiError, fmt.Errorf("android_settings.certificates[%d]: subject_name is required", i)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Seems like a good idea...
| ORDER BY certificate_templates.id ASC | ||
| LIMIT ? OFFSET ? | ||
| `, teamID, perPage+1, perPage*page, |
There was a problem hiding this comment.
nit, we have fleet.ListOptions and appendListOptionsWithCursorToSQL() to streamline pagination, I only know about this because Lucas gave me the same comment when I did the same thing 😆 . See ListSecretVariables for example. Not worth blocking over but good to keep in mind.
There was a problem hiding this comment.
hmm, I didn't know that method existed. But do we still need to implement the full text match in the where clause?
| const argsCountInsertCertificate = 4 | ||
|
|
||
| const sqlInsertCertificate = ` | ||
| INSERT INTO certificate_templates ( | ||
| name, | ||
| team_id, | ||
| certificate_authority_id, | ||
| subject_name | ||
| ) VALUES %s | ||
| ON DUPLICATE KEY UPDATE | ||
| name = VALUES(name), | ||
| team_id = VALUES(team_id), | ||
| certificate_authority_id = VALUES(certificate_authority_id), | ||
| subject_name = VALUES(subject_name) | ||
| ` | ||
|
|
||
| var placeholders strings.Builder | ||
| args := make([]interface{}, 0, len(certificateTemplates)*argsCountInsertCertificate) | ||
|
|
||
| for _, cert := range certificateTemplates { | ||
| args = append(args, cert.Name, cert.TeamID, cert.CertificateAuthorityID, cert.SubjectName) | ||
| placeholders.WriteString("(?,?,?,?),") | ||
| } | ||
|
|
||
| stmt := fmt.Sprintf(sqlInsertCertificate, strings.TrimSuffix(placeholders.String(), ",")) | ||
|
|
||
| if _, err := ds.writer(ctx).ExecContext(ctx, stmt, args...); err != nil { | ||
| return ctxerr.Wrap(ctx, err, "upserting certificate_templates") | ||
| } |
There was a problem hiding this comment.
nit: If we add db tags to the CertificateTemplate struct type, we can use NamedExecContext here, something like:
const sqlInsertCertificate = `
INSERT INTO certificate_templates (
name,
team_id,
certificate_authority_id,
subject_name
) VALUES (:name, :team, :certificate_authority_id, :subject_name)
ON DUPLICATE KEY UPDATE
name = VALUES(name),
team_id = VALUES(team_id),
certificate_authority_id = VALUES(certificate_authority_id),
subject_name = VALUES(subject_name)
`
if _, err := ds.writer(ctx).NamedExecContext(ctx, sqlInsertCertificate, certificateTemplates); err != nil {
return ctxerr.Wrap(ctx, err, "upserting certificate_templates")
}| func (ds *Datastore) BatchDeleteCertificateTemplates(ctx context.Context, certificateTemplateIDs []uint) error { | ||
| if len(certificateTemplateIDs) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| const sqlDeleteCertificateTemplates = ` | ||
| DELETE FROM certificate_templates | ||
| WHERE id IN (%s) | ||
| ` | ||
| var placeholders strings.Builder | ||
| args := make([]interface{}, 0, len(certificateTemplateIDs)) | ||
|
|
||
| for _, id := range certificateTemplateIDs { | ||
| args = append(args, id) | ||
| placeholders.WriteString("?,") | ||
| } | ||
|
|
||
| stmt := fmt.Sprintf(sqlDeleteCertificateTemplates, strings.TrimSuffix(placeholders.String(), ",")) | ||
|
|
||
| if _, err := ds.writer(ctx).ExecContext(ctx, stmt, args...); err != nil { | ||
| return ctxerr.Wrap(ctx, err, "deleting certificate_templates") | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
I think this is gated where needed at the endpoint level?
fleet/server/service/certificates.go
Lines 321 to 323 in b29b890
| return listCertificateTemplatesResponse{Certificates: certificates, Meta: paginationMetaData}, nil | ||
| } | ||
|
|
||
| func (svc *Service) ListCertificateTemplates(ctx context.Context, teamID uint, page int, perPage int) ([]*fleet.CertificateTemplateResponseSummary, *fleet.PaginationMetadata, error) { |
There was a problem hiding this comment.
same nit as above re: using fleet.ListOptions
There was a problem hiding this comment.
The reason I didn't want to use ListOptions, is because then I have to implement match queries and order by. : /
| if certificate.TeamID != 0 && (host.TeamID == nil || *host.TeamID != certificate.TeamID) { | ||
| return nil, fleet.NewPermissionError("host does not have access to this certificate template") | ||
| } |
There was a problem hiding this comment.
Are "global" certificate templates available to all teams? If not, then seems like we need to update this slightly so that it's "either certificate.TeamID = 0 and host.TeamID is nil, OR host.TeamID matches certificate.TeamID"
| return nil, nil, err | ||
| } | ||
|
|
||
| if !incoming.IsNoTeam() { |
There was a problem hiding this comment.
No certs for "no team" devices?
There was a problem hiding this comment.
I honestly don't know the answer to this. @marko-lisica or @getvictor, do we want to support devices on "no team"?
There was a problem hiding this comment.
Yes, we do want to support Android hosts and certs on No team.
There was a problem hiding this comment.
Yes, all "OS settings" are available for "No team", including certificates.
| if config.Controls.AndroidSettings == nil { | ||
| return nil | ||
| } | ||
|
|
||
| androidSettings, ok := config.Controls.AndroidSettings.(fleet.AndroidSettings) | ||
| if !ok { | ||
| return nil | ||
| } | ||
|
|
||
| if !androidSettings.Certificates.Valid || len(androidSettings.Certificates.Value) == 0 { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
I don't think we can take these shortcuts because we need to be able to clear out certs by having an empty certificates: key or even omitting that or the android_settings keys.
|
I'm gonna push up a new schema.sql and then merge this. I have a patch for the GitOps issue I mentioned above here that I can put in a separate PR (or add to mine), and we need another PR for generate-gitops, but in the interest of unblocking ongoing work we should get this code in. |
|
Todos to be addressed in separate PRs:
|
4 participants
Related issue: Resolves #35460, #35462
Checklist for submitter
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements)Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
Database migrations
COLLATE utf8mb4_unicode_ci).Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.