MF-1676 - Make Bootstrap fetch Certs data on demand #1669

arvindh123 · 2022-11-18T05:40:54Z

What does this do?

Renew the expired cert, If the cert is not available in bootstrap and thing service, then it will remove all the certs.

Which issue(s) does this PR fix/relate to?

Resolves #1677 and partially #1678.

List any changes that modify/break current functionality

Have you included tests for your changes?

Did you document any new/modified functionality?

Notes

arvindh123 · 2022-11-18T06:08:50Z

@mainflux/maintainers
Hello,
At present, in certs table, one thing can have Mutiple certificate, in cert table one thing will Mutiple expired certificate,
So if this PR tries to renew all the expired certificate of one thing, then there will be lot of certificates for a thing,
So to solve this problem, this PR is approach in different way, One thing can have one certificate.
The cert service will renew expired thing certificate and update the serial number in cert table.

dborovcanin · 2022-11-18T09:02:02Z

cmd/certs/main.go

+	defESURL          = "localhost:6379"
+	defESPass         = ""
+	defESDB           = "0"


Looks like these are not used.

Still Work in Progress for this variables, ES is used to get notification of deleted things

removed unsed

dborovcanin · 2022-11-18T09:05:08Z

certs/bootstrap/certs.go

+	return c.token, nil
+}
+
+func (c *bootstrapClient) UpdateCerts(ctx context.Context, thingID, clientCert, clientKey, caCert string) error {


Why don't we use c.sdk.UpdateBootstrapCerts here?

Okay I will try to use c.sdk.UpdateBootstrapCerts

@dborovcanin, we can't use c.sdk.UpdateBootstrapCerts , since SDK simply reply with status , and we need more detailed about the response body
https://github.com/mainflux/mainflux/blob/d73a5d53fec0313a51ca933321ea75de7ad0c90e/pkg/sdk/go/bootstrap.go#L180

@dborovcanin, we can't use c.sdk.UpdateBootstrapCerts , since SDK simply reply with status , and we need more detailed about the response body

https://github.com/mainflux/mainflux/blob/d73a5d53fec0313a51ca933321ea75de7ad0c90e/pkg/sdk/go/bootstrap.go#L180

Is this only because SDK does not return return error based on the reponse, but on status code? In that case, we should update SDK to return more specifc error.

What about this?

yes it use sdk now

dborovcanin · 2022-11-18T09:06:42Z

certs/certs.go

+	ListExpiredCerts(ctx context.Context, timeBefore time.Duration, limit, offset uint64) (Page, error)
+
+	// AutoRetrieveByThingID  retrieves list of certificate for a given thing ID without owner ID , used  for AutoRenew process
+	AutoRetrieveByThingID(ctx context.Context, thingID string) (Cert, error)


No need for the Auto prefix, it sounds misleading (I know we don't take owner ID into account, but that's fine because we do not have another repo method that does).

The repo method RetrieveByThing do the same but with owner ID

RetrieveByThing(ctx context.Context, ownerID, thingID string, offset, limit uint64) (Page, error)

So to identify the function with name I kept like that way. I can't able to think for another name at that time , Could you suggest a better name or prefix ?

You can make it a single method (RetrieveByThingID) and make it use userID only if present (i.e. ignore if empty, the same way search and filtering by some criteria usually work). On the service level, you decide how to handle authorization.

the AutoRetrieveByThingID functions are no more need , the function are removed
the AutoRemoveByThingID function name changed to ThingCertsRevokeHandler

dborovcanin · 2022-11-18T09:07:12Z

certs/postgres/certs.go

@@ -102,6 +102,31 @@ func (cr certsRepository) Save(ctx context.Context, cert certs.Cert) (string, er
 	return cert.Serial, nil
 }

+func (cr certsRepository) Update(ctx context.Context, cert certs.Cert) error {
+	q := `UPDATE certs SET serial = :serial, expire = :expire	WHERE thing_id = :thing_id AND owner_id = :owner_id`


Fix formatting.

I hope, I fixed the formatting

dborovcanin · 2022-11-18T09:08:51Z

certs/postgres/certs.go

@@ -170,6 +195,77 @@ func (cr certsRepository) RetrieveBySerial(ctx context.Context, ownerID, serialI
 	return c, nil
 }

+func (cr certsRepository) ListExpiredCerts(ctx context.Context, timeBefore time.Duration, limit, offset uint64) (certs.Page, error) {
+	whereClause := fmt.Sprintf("WHERE expire <= now()  - interval '%s'", timeBefore)


Are you sure using Database server time is better than using service time? It's possible that those two are not the same and this can be a source of the sneaky errors.

Okay I agree with your point, I will change the approch

This one doesn't seem addressed.

Done, expire calculate from service time

dborovcanin · 2022-11-18T09:10:53Z

certs/service.go

+	// Renew the exipred certificate from certs repo
+	RenewCerts(ctx context.Context, bsUpdateRenewCert bool) error
+
+	// Automattically trigger RenewCert function for given renew interval time


Fix typo Automatically and add a comment for the next method.

No more this function is needed in code. It was removed

Signed-off-by: dusanb94 <[email protected]> Signed-off-by: dusanb94 <[email protected]> Signed-off-by: Arvindh <[email protected]>

Signed-off-by: Arvindh <[email protected]>

dborovcanin · 2022-11-21T12:31:23Z

certs/api/logging.go

@@ -90,3 +90,42 @@ func (lm *loggingMiddleware) RevokeCert(ctx context.Context, token, thingID stri

 	return lm.svc.RevokeCert(ctx, token, thingID)
 }
+
+func (lm *loggingMiddleware) RenewCerts(ctx context.Context, bsUpdateRenewCert bool) (err error) {


Please align the log message with the rest of the service: Method <method name> <other useful info> took <duration> to complete <errors if any>.

The method returns only error
The error contains all the details,

dborovcanin · 2022-11-21T12:32:31Z

certs/bootstrap/certs.go

+	// ErrUnexpectedBSResponse indicates unexpected response from Bootstrap service.
+	ErrUnexpectedBSResponse = errors.New("unexpected Bootstrap service response")
+
+	ErrUnableToAccess = errors.New("unable to access bootstrap service")


Add comments for exported errors.

Please don't forget to address this one.

dborovcanin · 2022-11-21T12:33:04Z

certs/bootstrap/certs.go

+	return c.token, nil
+}
+
+func (c *bootstrapClient) UpdateCerts(ctx context.Context, thingID, clientCert, clientKey, caCert string) error {


What about this?

dborovcanin · 2022-11-21T12:33:46Z

certs/bootstrap/certs.go

+	}
+	return errors.New(string(b))
+}
+func request(ctx context.Context, method, url string, data []byte, header map[string]string) (*http.Response, error) {


Add an empty line between methods.

dborovcanin · 2022-11-21T12:34:06Z

certs/certs.go

+	// 	// AutoRetrieveByThingID  retrieves list of certificate for a given thing ID without owner ID , used  for AutoRenew process
+	// 	AutoRetrieveByThingID(ctx context.Context, thingID string) (Cert, error)
+
+	// // AutoRemoveByThingID removes certificate from DB for a given thing ID without owner ID , used  for AutoRenew process
+	// AutoRemoveByThingID(ctx context.Context, thingID string) error


Remove commented code.

The functions no more needed removed

dborovcanin · 2022-11-21T12:35:54Z

certs/postgres/certs.go

@@ -170,6 +195,77 @@ func (cr certsRepository) RetrieveBySerial(ctx context.Context, ownerID, serialI
 	return c, nil
 }

+func (cr certsRepository) ListExpiredCerts(ctx context.Context, timeBefore time.Duration, limit, offset uint64) (certs.Page, error) {
+	whereClause := fmt.Sprintf("WHERE expire <= now()  - interval '%s'", timeBefore)


This one doesn't seem addressed.

dborovcanin · 2022-11-21T12:36:20Z

certs/postgres/certs.go

+// func (cr certsRepository) AutoRetrieveByThingID(ctx context.Context, thingID string) (certs.Cert, error) {
+// 	q := `SELECT thing_id, owner_id, serial, expire FROM certs WHERE thing_id = $1`
+// 	var dbcrt dbCert
+// 	var c certs.Cert
+// 	if err := cr.db.QueryRowxContext(ctx, q, thingID).StructScan(&dbcrt); err != nil {
+// 		pqErr, ok := err.(*pq.Error)
+// 		if err == sql.ErrNoRows || ok && errInvalid == pqErr.Code.Name() {
+// 			return c, errors.Wrap(errors.ErrNotFound, err)
+// 		}
+
+// 		return c, errors.Wrap(errors.ErrViewEntity, err)
+// 	}
+// 	c = toCert(dbcrt)
+// 	return c, nil
+// }
+
+// func (cr certsRepository) AutoRemoveByThingID(ctx context.Context, thingID string) error {
+// 	if _, err := cr.AutoRetrieveByThingID(ctx, thingID); err != nil {
+// 		return errors.Wrap(errors.ErrRemoveEntity, err)
+// 	}
+// 	q := `DELETE FROM certs WHERE thing_id = :thingID`
+// 	var c certs.Cert
+// 	c.ThingID = thingID
+// 	dbcrt := toDBCert(c)
+// 	if _, err := cr.db.NamedExecContext(ctx, q, dbcrt); err != nil {
+// 		return errors.Wrap(errors.ErrRemoveEntity, err)
+// 	}
+// 	return nil
+// }
+


Remove commented code.

dborovcanin · 2022-11-21T12:37:27Z

certs/redis/consumer/event.go

+type updateChannelEvent struct {
+	id       string
+	name     string
+	metadata map[string]interface{}
+}


dborovcanin · 2022-11-21T12:37:58Z

certs/service.go

-	ErrFailedCertRevocation = errors.New("failed to revoke certificate")
+	ErrFailedCertRevocation = errors.New("failed to revoke certificate in PKI")
+
+	errThingNotInBS = errors.New("Thing not found in bootstrap service")


Comment exported errors.

Signed-off-by: Arvindh <[email protected]>

dborovcanin requested changes Nov 18, 2022

View reviewed changes

dborovcanin and others added 15 commits November 21, 2022 13:09

Update Go version and dependencies (absmach#1663)

e4bddb1

Signed-off-by: dusanb94 <[email protected]> Signed-off-by: dusanb94 <[email protected]> Signed-off-by: Arvindh <[email protected]>

Add : auto renew certficate

0d2c639

Signed-off-by: Arvindh <[email protected]>

Add : redis event stream and bug fix

0e9be3f

Signed-off-by: Arvindh <[email protected]>

Add : empty mock

786e2b5

Signed-off-by: Arvindh <[email protected]>

Add : logging and metrics

3a6a902

Signed-off-by: Arvindh <[email protected]>

Fix : certs BSClient

716d1e8

Signed-off-by: Arvindh <[email protected]>

add: bsclient in service_test

85da2b2

Signed-off-by: Arvindh <[email protected]>

add: bsclient in service

c6fd44c

Signed-off-by: Arvindh <[email protected]>

add: bsclient default values

f2d0e62

Signed-off-by: Arvindh <[email protected]>

fix: bsclient in service_test

765f2f5

Signed-off-by: Arvindh <[email protected]>

fix: env names

ba92450

Signed-off-by: Arvindh <[email protected]>

fix: env names

8f0b5e7

Signed-off-by: Arvindh <[email protected]>

fix: bsclient

6fd08c2

Signed-off-by: Arvindh <[email protected]>

fix: comments typo

526e7ce

Signed-off-by: Arvindh <[email protected]>

change: things with multi cert

a60bab3

Signed-off-by: Arvindh <[email protected]>

arvindh123 force-pushed the certs-auto-renew branch from 965534c to a60bab3 Compare November 21, 2022 07:39

arvindh123 added 3 commits November 21, 2022 17:24

fix: cert query

c3790bc

Signed-off-by: Arvindh <[email protected]>

add : multierror

46a7685

Signed-off-by: Arvindh <[email protected]>

remove: unused variables

56e146a

Signed-off-by: Arvindh <[email protected]>

dborovcanin requested changes Nov 21, 2022

View reviewed changes

arvindh123 and others added 8 commits November 21, 2022 18:18

remove: AutRenew function from service

1f42221

Signed-off-by: Arvindh <[email protected]>

remove: unused commented function

e3b0da2

Signed-off-by: Arvindh <[email protected]>

remove: unused commented function

1ca7630

Signed-off-by: Arvindh <[email protected]>

add: comment to exported variable

f44b428

Signed-off-by: Arvindh <[email protected]>

remove: AutRenew function from service

2929401

Signed-off-by: Arvindh <[email protected]>

remove: AutRenew function from service

47dd5d9

Signed-off-by: Arvindh <[email protected]>

remove: multierror in pkg

031d2af

Signed-off-by: Arvindh <[email protected]>

Merge branch 'master' into certs-auto-renew

f9dc560

arvindh123 added 2 commits December 14, 2022 19:32

Merge branch 'master' into certs-auto-renew

ffe822c

Merge branch 'master' into certs-auto-renew

6928b84

dborovcanin changed the title ~~MF-1668: Auto renewal of expired certificates from the PKI issuer~~ MF-1668 - Auto renewal of expired certificates from the PKI issuer Jan 4, 2023

arvindh123 added 7 commits January 9, 2023 22:30

draft certs logic modifed

56ee1d6

Signed-off-by: Arvindh <[email protected]>

add things event stream client

8a83e88

Signed-off-by: Arvindh <[email protected]>

Merge branch 'master' into certs-auto-renew

e922aa9

add certs

6a13247

Signed-off-by: Arvindh <[email protected]>

rebase with mainflux/master

85d9794

Signed-off-by: Arvindh <[email protected]>

rebase with mainflux/master

a370c4b

Signed-off-by: Arvindh <[email protected]>

add certs

ef7205f

Signed-off-by: Arvindh <[email protected]>

arvindh123 changed the title ~~MF-1668 - Auto renewal of expired certificates from the PKI issuer~~ MF-1676 Make Bootstrap fetch Certs data on demand & MF - 1677 Enable multiple certificates for a single Thing Jan 17, 2023

arvindh123 added 7 commits January 17, 2023 13:54

remove : unused variable

ebcbe2d

Signed-off-by: Arvindh <[email protected]>

update: certs postgres migration

ef84ebf

Signed-off-by: Arvindh <[email protected]>

fix: IssueCert

cc1f369

Signed-off-by: Arvindh <[email protected]>

add: test for Issue certificate

e41f56e

Signed-off-by: Arvindh <[email protected]>

fix: cert issue endpoint

22a5d3f

Signed-off-by: Arvindh <[email protected]>

fix: cert issue endpoint

35b662c

Signed-off-by: Arvindh <[email protected]>

fix: cert CRUD endpoints

32f409e

Signed-off-by: Arvindh <[email protected]>

dborovcanin changed the title ~~MF-1676 Make Bootstrap fetch Certs data on demand & MF - 1677 Enable multiple certificates for a single Thing~~ MF-1676 - Make Bootstrap fetch Certs data on demand Jan 25, 2023

arvindh123 added 7 commits February 9, 2023 15:06

Merge branch 'master' into certs-auto-renew

ebd72d2

Signed-off-by: Arvindh <[email protected]>

Update renew, revoke remove certificate by thing id

7841d81

Signed-off-by: Arvindh <[email protected]>

Update event handers

98cd2ad

Signed-off-by: Arvindh <[email protected]>

Update event handers

310d18d

Signed-off-by: Arvindh <[email protected]>

Update event handers

1457353

Signed-off-by: Arvindh <[email protected]>

add: certs api spec

824ca8a

Signed-off-by: Arvindh <[email protected]>

add: certs api spec

26c24a8

Signed-off-by: Arvindh <[email protected]>

arvindh123 mentioned this pull request Jul 28, 2023

NOISSUE - Return certs on bootstrap view response #1855

Merged

dborovcanin assigned arvindh123 Jul 2, 2024

dborovcanin added enhancement later labels Jul 2, 2024

MF-1676 - Make Bootstrap fetch Certs data on demand #1669

Are you sure you want to change the base?

MF-1676 - Make Bootstrap fetch Certs data on demand #1669

Conversation

arvindh123 commented Nov 18, 2022 • edited by dborovcanin Loading

What does this do?

Which issue(s) does this PR fix/relate to?

List any changes that modify/break current functionality

Have you included tests for your changes?

Did you document any new/modified functionality?

Notes

arvindh123 commented Nov 18, 2022

Choose a reason for hiding this comment

arvindh123 Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arvindh123 commented Nov 18, 2022 •

edited by dborovcanin

Loading

arvindh123 Nov 18, 2022 •

edited

Loading