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

MF-1179 - Add a certificate service and certs endpoint to SDK #1188

Merged
merged 83 commits into from
Jul 21, 2020

Conversation

mteodor
Copy link
Contributor

@mteodor mteodor commented Jun 1, 2020

Signed-off-by: Mirko Teodorovic mirko.teodorovic@gmail.com

Resolves #1179

Copy link
Contributor

@drasko drasko left a comment

Choose a reason for hiding this comment

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

This will demand also documentation about new features.

@mteodor mteodor force-pushed the add-certs branch 2 times, most recently from 0899715 to dfa6fc3 Compare June 5, 2020 12:35
@mteodor mteodor marked this pull request as ready for review June 5, 2020 13:40
@mteodor mteodor requested a review from a team as a code owner June 5, 2020 13:40
pkg/sdk/go/certs.go Outdated Show resolved Hide resolved
provision/config.go Outdated Show resolved Hide resolved
provision/api/requests.go Outdated Show resolved Hide resolved
.env Outdated
MF_PROVISION_CERTS_CA=/etc/ssl/certs/ca.crt
MF_PROVISION_CERTS_CA_KEY=/etc/ssl/certs/ca.key
MF_PROVISION_CERTS_RSA_BITS=4096
MF_PROVISION_CERTS_DAYS_VALID=2400h
Copy link
Contributor

Choose a reason for hiding this comment

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

ENV is called DAYS, but you put hours?

@@ -39,7 +39,7 @@ type BootstrapConfig struct {
State int `json:"state,omitempty"`
}

func (sdk mfSDK) AddBootstrap(token string, cfg BootstrapConfig) (string, error) {
func (sdk *mfSDK) AddBootstrap(token string, cfg BootstrapConfig) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

SDK is mutable? Something is probably strange...

@drasko
Copy link
Contributor

drasko commented Jun 7, 2020

CI is failing, @mteodor please take a look at this as well.

@drasko
Copy link
Contributor

drasko commented Jun 10, 2020

@mteodor what do we do with this PR? Are you closing or moving it to draft for modifications?

@mteodor mteodor marked this pull request as draft June 11, 2020 10:57
certs/service.go Outdated
return "", "", errors.Wrap(ErrFailedCertCreation, ErrRsaBitsValueWrong)
}
var priv interface{}
// p224 := elliptic.P224()
Copy link
Contributor

Choose a reason for hiding this comment

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

Kill dead code

@@ -35,3 +35,24 @@ func doProvision(svc provision.Service) endpoint.Endpoint {

}
}

func doCert(svc provision.Service) endpoint.Endpoint {
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this function do? Creates Cert? If yes, consider calling it createCert(). Or just Cert().


code := m.Run()

// defers will not be run when using os.Exit
Copy link
Contributor

Choose a reason for hiding this comment

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

Comments start with capital letter


func (ms *metricsMiddleware) IssueCert(ctx context.Context, token, thingID string, daysValid string, keyBits int, keyType string) (certs.Cert, error) {
defer func(begin time.Time) {
ms.counter.With("method", "issue").Add(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Use issue_cert here

@mteodor mteodor marked this pull request as ready for review June 30, 2020 16:02
.env Outdated
@@ -116,13 +115,44 @@ MF_PROVISION_THINGS_LOCATION=http://things:8182
MF_PROVISION_USER=
MF_PROVISION_PASS=
MF_PROVISION_API_KEY=
MF_PROVISION_CERTS_SVC_URL=http://localhost/certs
MF_PROVISION_CERTS_SVC_URL=
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we let a default value?


var (
errSaveDB = errors.New("failed to save certs to database")
errRetrieve = errors.New("failed to retrieve certs from database")
Copy link
Contributor

Choose a reason for hiding this comment

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

You have 2 white space in this log. I would maybe use failed to read certs from database

@mteodor mteodor changed the title MF-1179 - adding certificate endpoint MF-1179 - adding certificate service Jul 1, 2020
@codecov-commenter
Copy link

codecov-commenter commented Jul 1, 2020

Codecov Report

Merging #1188 into master will decrease coverage by 1.11%.
The diff coverage is 17.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1188      +/-   ##
==========================================
- Coverage   77.44%   76.32%   -1.12%     
==========================================
  Files         104      106       +2     
  Lines        6875     6909      +34     
==========================================
- Hits         5324     5273      -51     
- Misses       1164     1253      +89     
+ Partials      387      383       -4     
Impacted Files Coverage Δ
certs/postgres/certs.go 0.00% <0.00%> (ø)
pkg/sdk/go/certs.go 0.00% <0.00%> (ø)
pkg/sdk/go/sdk.go 96.55% <ø> (ø)
provision/api/endpoint.go 0.00% <0.00%> (ø)
provision/api/logging.go 0.00% <0.00%> (ø)
provision/api/transport.go 0.00% <0.00%> (ø)
things/api/things/http/transport.go 90.72% <0.00%> (-1.27%) ⬇️
things/redis/channels.go 73.91% <ø> (ø)
certs/postgres/init.go 77.41% <77.41%> (ø)
pkg/sdk/go/message.go 35.55% <100.00%> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be13613...3670b3c. Read the comment docs.

@dborovcanin dborovcanin changed the title MF-1179 - adding certificate service MF-1179 - Add a certificate service and certs endpoint to SDK Jul 1, 2020
if err := req.validate(); err != nil {
return nil, err
}
fmt.Println(fmt.Sprintf("request %v", req))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove debug code.

}

return svc.RevokeCert(ctx, req.token, req.ThingID, req.CertSerial)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove an empty line.


func (lm *loggingMiddleware) RevokeCert(ctx context.Context, token, thingID, certSerial string) (c certs.Revoke, err error) {
defer func(begin time.Time) {
message := fmt.Sprintf("Method revoke_certificates for token: %s and thing: %s took %s to complete", token, thingID, time.Since(begin))
Copy link
Collaborator

Choose a reason for hiding this comment

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

"revoke_cert"

Comment on lines 46 to 48
if thingID == "" {
return certs.Page{}, errEmptyThingID
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can move this check to the service layer.

thing_id TEXT NOT NULL,
expire TIMESTAMPTZ NOT NULL,
serial TEXT NOT NULL,
PRIMARY KEY (thing_id, serial)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can have multiple certificates for one Thing. If there are no practical examples where this can be useful, I would stick with one cert per Thing.

certs/service.go Show resolved Hide resolved
certs/service.go Outdated
certsRepo Repository
sdk mfsdk.SDK
conf Config
PKIClient *api.Client
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we create a wrapper around this client and use it as a lib? Move custom requests and responses to a separate package and import it in certs service? Also, wrap HTTP requests and responses to make it transparent from the service layer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mteodor Can you, please, take a look at this remark?

certs/service.go Outdated
Comment on lines 273 to 172
r := cs.PKIClient.NewRequest("POST", cs.getRevokeURL())
if err := r.SetJSONBody(cReq); err != nil {
return Revoke{}, err
}

resp, err := cs.PKIClient.RawRequest(r)
if resp != nil {
defer resp.Body.Close()
}

if err != nil {
return Revoke{}, err
}

if resp.StatusCode >= http.StatusBadRequest {
_, err := ioutil.ReadAll(resp.Body)
if err != nil {
return Revoke{}, err
}
return Revoke{}, errors.Wrap(ErrFailedCertCreation, err)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

For example, this code would be in the Vault client wrapper.

}
}

func revokeCertificate(svc certs.Service) endpoint.Endpoint {
Copy link
Contributor

Choose a reason for hiding this comment

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

revokeCert

}
}

func listCertificates(svc certs.Service) endpoint.Endpoint {
Copy link
Contributor

Choose a reason for hiding this comment

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

listCerts

return lm.svc.IssueCert(ctx, token, thingID, daysValid, keyBits, keyType)
}

func (lm *loggingMiddleware) ListCertificates(ctx context.Context, token, thingID string, offset, limit uint64) (cp certs.Page, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ListCerts

Comment on lines 81 to 82
q := `INSERT INTO certs (thing_id, serial, expire)
VALUES (:thing_id, :serial, :expire)`
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO No need for 2 lines here

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Comment on lines 161 to 167
func (p *pkiAgent) getIssueURL() string {
return "/" + apiVer + "/" + p.path + "/" + issue + "/" + p.role
}

func (p *pkiAgent) getRevokeURL() string {
return "/" + apiVer + "/" + p.path + "/" + revoke
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use these as fields in pkiAgent? Can we set okiAgent.issueURL and pkiAgent.revokeURL in the NewVaultClient method?

// Copyright (c) Mainflux
// SPDX-License-Identifier: Apache-2.0

// Package postgres contains repository implementations using PostgreSQL as
Copy link
Collaborator

@dborovcanin dborovcanin Jul 16, 2020

Choose a reason for hiding this comment

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

Wrong package description.

certs/pki/pki.go Show resolved Hide resolved
certs/api/requests.go Show resolved Hide resolved
)

func TestIssueCert(t *testing.T) {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Either delete file or add a test.

certs/swagger.yml Show resolved Hide resolved
MF_CERTS_VAULT_ROLE: ${MF_CERTS_VAULT_ROLE}
volumes:
- ../../ssl/certs/ca.key:/etc/ssl/certs/ca.key
- ../../ssl/certs/ca.crt:/etc/ssl/certs/ca.crt
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add an empty line at the end of the file.

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Copy link
Contributor

@manuio manuio left a comment

Choose a reason for hiding this comment

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

LGTM

@manuio manuio merged commit b910244 into absmach:master Jul 21, 2020
manuio pushed a commit that referenced this pull request Oct 12, 2020
* adding certificate issuing

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* adding cert endpoint

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* update envs

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* update envs

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* move certs creation to sdk

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* move certs creation to sdk

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* move certs creation to sdk

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* fix env vars

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* add comment

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* update sdk

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* fix vars

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* add volumes

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* fix merge config for int

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* remove env

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* fix error handling

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* add cert test, change receiver to pointer

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* add docs

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* fix var naming

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* correct error naming

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* adding certs service

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* add certs

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* add certs

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* change func receiever

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* add default cert issue method

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* add config

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* small fix

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* remove some testing code

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* add cert issue

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* add vault api client

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* additional endpoints

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* add swagger for certs

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* remove certs from provision

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* clean provision from certs

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* add list certificates endpoint

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* add vault api in vendor

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* add certs

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* add revoke, fix bugs

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* fix sdk for certs

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* minor changes, add env, doc

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* minor changes, add env, doc

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* minor changes, add env, doc

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* small changes

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* remove CA for signing from provision

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* add docker file for certs

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* fix mock sdk

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* add line

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* fix RevokeCert

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* renam ENV

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* remove tests temporarily

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* fix naming

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* renam vars

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* add cli for issue cert

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* add cli for issue cert

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* add cli for issue cert

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* add cli for issue cert

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* remove not needed envs

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* fix linter errors, add cli

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* fix linter errors, add cli, var rename

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* fix reviews, add viewcert, fix view all certs

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* remove view cert, as it will be retrieved from PKI

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* change endpoints

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* add default env val

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* remove some errors

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* refactor, make wrapper lib for vault

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* refactor, make wrapper lib for vault

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* refactor, make wrapper lib for vault

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* fix revoking

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* refactor, make wrapper lib for vault

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* update vendor

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* fix comment

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* add comments

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* remove unused

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* remove unused field

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* update vendor

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* refactor pki

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* refactor pki

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* refactor pki, update vendor

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* refactor pki

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* fix comment

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* minor fix

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* remove methods, use fields

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* fix comments and package desc

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>

* fix comments and package desc

Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
@mteodor mteodor deleted the add-certs branch September 13, 2021 12:27
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.

Add a certificate service and certs endpoint to SDK
5 participants