Device claiming client #1392

johanstokking · 2019-09-26T20:49:15Z

Summary

Client for end device claiming and some necessary plumbing

References https://github.com/thethingsindustries/lorawan-stack/issues/1673
References #138

Changes

Add CLI commands for device claiming, including application authorization
Remove nil-checks in NS device registry
Change claim authentication code to string, also fixed for TR005 QR code parsing

Notes for Reviewers

@rvolosatovs let me know why we need(ed) those nil-checks in NS

Release Notes

Added support for claiming end devices through CLI. See ttn-lw-cli end-device claim --help for more information
Added support generating QR codes for claiming. See ttn-lw-cli end-device generate-qr --help for more information
Added support for authorizing device claiming on application level through CLI. See ttn-lw-cli application claim authorize --help for more information

coveralls · 2019-09-26T21:26:43Z

Coverage increased (+0.03%) to 73.199% when pulling 03d2b58 on feature/end-device-claiming into ae469dc on master.

rvolosatovs · 2019-09-29T10:29:57Z

pkg/networkserver/grpc_deviceregistry.go

@@ -134,22 +134,6 @@ func validABPSessionKey(key *ttnpb.KeyEnvelope) bool {

 // Set implements NsEndDeviceRegistryServer.
 func (ns *NetworkServer) Set(ctx context.Context, req *ttnpb.SetEndDeviceRequest) (*ttnpb.EndDevice, error) {
-	if ttnpb.HasAnyField(req.FieldMask.Paths, "session.dev_addr") && (req.EndDevice.Session == nil || req.EndDevice.Session.DevAddr.IsZero()) {


The only reason why someone would want to set the session.dev_addr to zero value is to make the device model corrupted. I don't think we should allow this.
The set transaction should fail anyway due to invalid dev_addr, but we can(and should) just directly send a clear error to the user without even hitting the stores or doing anything else

The field mask indicate what the caller knows about the entity being passed.

If session == nil, a field mask containing session.dev_addr is not wrong. It should be ineffective.

Sure, NS should avoid invalid state. And a session.dev_addr that is zero is invalid state, so that should be checked, but there's no need to return an error if session == nil.

I assume you mean session == nil on the stored device. The solution is simple though - we should get the session if session.dev_addr is requested and do different checks in update and create functionality

I assume you mean session == nil on the stored device.

No, on the given end device. Please see revised change in 76ad7a7

If session == nil, a field mask containing session.dev_addr is not wrong. It should be ineffective.

That is not true - if session == nil, but session.dev_addr is specified - the session.dev_addr on the stored device is deleted(assuming session.dev_addr is stored)

That is not true - if session == nil, but session.dev_addr is specified - the session.dev_addr on the stored device is deleted(assuming session.dev_addr is stored)

Ineffective was specifically for create. For update, again, the field mask specifies what the caller knows. I.e. the caller knows session.dev_addr and session == nil, so the entire session should be reset.

That's not how fieldmasks work, right?
If you specify a.b, a.c must not change.
From https://developers.google.com/protocol-buffers/docs/reference/java/com/google/protobuf/FieldMask:

A field mask in update operations specifies which fields of the
targeted resource are going to be updated. The API is required
to only change the values of the fields as specified in the mask
and leave the others untouched. If a resource is passed in to
describe the updated values, the API ignores the values of all
fields not covered by the mask.

Also, please note that the registry does not validate session.dev_addr set by the user (at least now) - that happens in this RPC.

Right, then make it ineffective. session will also be part of the field mask anyway.

rvolosatovs · 2019-09-29T10:30:10Z

pkg/networkserver/grpc_deviceregistry.go

-	if ttnpb.HasAnyField(req.FieldMask.Paths, "session.dev_addr") && (req.EndDevice.Session == nil || req.EndDevice.Session.DevAddr.IsZero()) {
-		return nil, errInvalidFieldValue.WithAttributes("field", "session.dev_addr")
-	}
-	if ttnpb.HasAnyField(req.FieldMask.Paths, "session.keys.session_key_id") && (req.EndDevice.Session == nil || len(req.EndDevice.Session.SessionKeys.GetSessionKeyID()) == 0) {


IDEM, same as DevAddr

The length check, yes, but not the session existence check

rvolosatovs · 2019-09-29T10:31:18Z

pkg/networkserver/grpc_deviceregistry.go

-	if ttnpb.HasAnyField(req.FieldMask.Paths, "session.keys.session_key_id") && (req.EndDevice.Session == nil || len(req.EndDevice.Session.SessionKeys.GetSessionKeyID()) == 0) {
-		return nil, errInvalidFieldValue.WithAttributes("field", "session.keys.session_key_id")
-	}
-	if ttnpb.HasAnyField(req.FieldMask.Paths, "session.keys.f_nwk_s_int_key.key") && (req.EndDevice.Session == nil || req.EndDevice.Session.SessionKeys.GetFNwkSIntKey().GetKey().IsZero()) {


IDEM, removing this field results in a corrupted session, which NS cannot use. We should not allow doing that - the user should just remove the session

Same, will recover the zero-check

pkg/networkserver/grpc_deviceregistry_test.go

cmd/ttn-lw-cli/commands/applications_claim.go

johanstokking · 2019-10-02T17:18:53Z

@rvolosatovs please review pkg/networkserver only

rvolosatovs · 2019-10-03T15:38:52Z

I think the session-related diff in NS grpc_deviceregistry.go should look something like this:

diff --git a/pkg/networkserver/grpc_deviceregistry.go b/pkg/networkserver/grpc_deviceregistry.go
index a79778834..894e2162d 100644
--- a/pkg/networkserver/grpc_deviceregistry.go
+++ b/pkg/networkserver/grpc_deviceregistry.go
@@ -128,28 +128,16 @@ func (ns *NetworkServer) Get(ctx context.Context, req *ttnpb.GetEndDeviceRequest
 	return dev, nil
 }
 
+func sessionKeyIsSet(key *ttnpb.KeyEnvelope) bool {
+	return key != nil && (len(key.EncryptedKey) > 0 || !key.Key.IsZero())
+}
+
 func validABPSessionKey(key *ttnpb.KeyEnvelope) bool {
 	return key != nil && key.KEKLabel == "" && !key.Key.IsZero()
 }
 
 // Set implements NsEndDeviceRegistryServer.
 func (ns *NetworkServer) Set(ctx context.Context, req *ttnpb.SetEndDeviceRequest) (*ttnpb.EndDevice, error) {
-	if ttnpb.HasAnyField(req.FieldMask.Paths, "session.dev_addr") && (req.EndDevice.Session == nil || req.EndDevice.Session.DevAddr.IsZero()) {
-		return nil, errInvalidFieldValue.WithAttributes("field", "session.dev_addr")
-	}
-	if ttnpb.HasAnyField(req.FieldMask.Paths, "session.keys.session_key_id") && (req.EndDevice.Session == nil || len(req.EndDevice.Session.SessionKeys.GetSessionKeyID()) == 0) {
-		return nil, errInvalidFieldValue.WithAttributes("field", "session.keys.session_key_id")
-	}
-	if ttnpb.HasAnyField(req.FieldMask.Paths, "session.keys.f_nwk_s_int_key.key") && (req.EndDevice.Session == nil || req.EndDevice.Session.SessionKeys.GetFNwkSIntKey().GetKey().IsZero()) {
-		return nil, errInvalidFieldValue.WithAttributes("field", "session.keys.f_nwk_s_int_key.key")
-	}
-	if ttnpb.HasAnyField(req.FieldMask.Paths, "session.keys.s_nwk_s_int_key.key") && (req.EndDevice.Session == nil || req.EndDevice.Session.SessionKeys.GetSNwkSIntKey().GetKey().IsZero()) {
-		return nil, errInvalidFieldValue.WithAttributes("field", "session.keys.s_nwk_s_int_key.key")
-	}
-	if ttnpb.HasAnyField(req.FieldMask.Paths, "session.keys.nwk_s_enc_key.key") && (req.EndDevice.Session == nil || req.EndDevice.Session.SessionKeys.GetNwkSEncKey().GetKey().IsZero()) {
-		return nil, errInvalidFieldValue.WithAttributes("field", "session.keys.nwk_s_enc_key.key")
-	}
-
 	if ttnpb.HasAnyField(req.FieldMask.Paths, "frequency_plan_id") && req.EndDevice.FrequencyPlanID == "" {
 		return nil, errInvalidFieldValue.WithAttributes("field", "frequency_plan_id")
 	}
@@ -184,6 +172,14 @@ func (ns *NetworkServer) Set(ctx context.Context, req *ttnpb.SetEndDeviceRequest
 			break
 		}
 	}
+	for _, p := range req.FieldMask.Paths {
+		if strings.HasPrefix(p, "session.") {
+			gets = append(gets,
+				"session",
+			)
+			break
+		}
+	}
 
 	var evt events.Event
 	var addDownlinkTask bool
@@ -204,7 +200,26 @@ func (ns *NetworkServer) Set(ctx context.Context, req *ttnpb.SetEndDeviceRequest
 			); err != nil {
 				return nil, nil, errInvalidFieldMask.WithCause(err)
 			}
-			if ttnpb.HasAnyField(req.FieldMask.Paths, "session.dev_addr") {
+
+			if dev.Session != nil {
+				if !dev.Session.DevAddr.IsZero() && ttnpb.HasAnyField(sets, "session.dev_addr") && (req.EndDevice.Session == nil || req.EndDevice.Session.DevAddr.IsZero()) {
+					return nil, nil, errInvalidFieldValue.WithAttributes("field", "session.dev_addr")
+				}
+				if len(dev.Session.SessionKeyID) > 0 && ttnpb.HasAnyField(sets, "session.keys.session_key_id") && (req.EndDevice.Session == nil || len(req.EndDevice.Session.SessionKeys.GetSessionKeyID()) == 0) {
+					return nil, nil, errInvalidFieldValue.WithAttributes("field", "session.keys.session_key_id")
+				}
+				if sessionKeyIsSet(dev.Session.SessionKeys.GetFNwkSIntKey()) && ttnpb.HasAnyField(sets, "session.keys.f_nwk_s_int_key.key") && (req.EndDevice.Session == nil || req.EndDevice.Session.SessionKeys.GetFNwkSIntKey().GetKey().IsZero()) {
+					return nil, nil, errInvalidFieldValue.WithAttributes("field", "session.keys.f_nwk_s_int_key.key")
+				}
+				if sessionKeyIsSet(dev.Session.SessionKeys.GetSNwkSIntKey()) && ttnpb.HasAnyField(sets, "session.keys.s_nwk_s_int_key.key") && (req.EndDevice.Session == nil || req.EndDevice.Session.SessionKeys.GetSNwkSIntKey().GetKey().IsZero()) {
+					return nil, nil, errInvalidFieldValue.WithAttributes("field", "session.keys.s_nwk_s_int_key.key")
+				}
+				if sessionKeyIsSet(dev.Session.SessionKeys.GetNwkSEncKey()) && ttnpb.HasAnyField(sets, "session.keys.nwk_s_enc_key.key") && (req.EndDevice.Session == nil || req.EndDevice.Session.SessionKeys.GetNwkSEncKey().GetKey().IsZero()) {
+					return nil, nil, errInvalidFieldValue.WithAttributes("field", "session.keys.nwk_s_enc_key.key")
+				}
+			}
+
+			if ttnpb.HasAnyField(sets, "session.dev_addr") && req.EndDevice.Session != nil && !req.EndDevice.Session.DevAddr.IsZero() {
 				req.EndDevice.DevAddr = &req.EndDevice.Session.DevAddr
 				sets = append(sets, "ids.dev_addr")
 			}
@@ -333,7 +348,7 @@ func (ns *NetworkServer) Set(ctx context.Context, req *ttnpb.SetEndDeviceRequest
 			)
 		}
 		if req.EndDevice.DevAddr != nil {
-			if !ttnpb.HasAnyField(req.FieldMask.Paths, "session.dev_addr") || !req.EndDevice.DevAddr.Equal(req.EndDevice.Session.DevAddr) {
+			if !ttnpb.HasAnyField(req.FieldMask.Paths, "session.dev_addr") || req.EndDevice.Session == nil || !req.EndDevice.DevAddr.Equal(req.EndDevice.Session.DevAddr) {
 				return nil, nil, errInvalidFieldValue.WithAttributes("field", "ids.dev_addr")
 			}
 		}
@@ -345,7 +360,7 @@ func (ns *NetworkServer) Set(ctx context.Context, req *ttnpb.SetEndDeviceRequest
 			if req.EndDevice.DevEUI == nil {
 				return nil, nil, errNoDevEUI
 			}
-			if !ttnpb.HasAnyField([]string{"session"}, sets...) {
+			if !ttnpb.HasAnyField([]string{"session"}, sets...) || req.EndDevice.Session == nil {
 				return &req.EndDevice, sets, nil
 			}
 		}
@@ -356,6 +371,9 @@ func (ns *NetworkServer) Set(ctx context.Context, req *ttnpb.SetEndDeviceRequest
 		); err != nil {
 			return nil, nil, errInvalidFieldMask.WithCause(err)
 		}
+		if req.EndDevice.Session == nil || req.EndDevice.Session.DevAddr.IsZero() {
+			return nil, nil, errInvalidFieldValue.WithAttributes("field", "session.dev_addr")
+		}
 		req.EndDevice.EndDeviceIdentifiers.DevAddr = &req.EndDevice.Session.DevAddr
 		sets = append(sets,
 			"ids.dev_addr",
@@ -381,11 +399,11 @@ func (ns *NetworkServer) Set(ctx context.Context, req *ttnpb.SetEndDeviceRequest
 				return nil, nil, errInvalidNwkSEncKey
 			}
 		} else {
-			if err := ttnpb.ProhibitFields(sets,
-				"session.keys.nwk_s_enc_key.key",
-				"session.keys.s_nwk_s_int_key.key",
-			); err != nil {
-				return nil, nil, errInvalidFieldMask.WithCause(err)
+			if ttnpb.HasAnyField([]string{"session.keys.nwk_s_enc_key"}, sets...) && req.EndDevice.Session.SessionKeys.NwkSEncKey != nil {
+				return nil, nil, errInvalidFieldValue.WithAttributes("field", "session.keys.nwk_s_enc_key")
+			}
+			if ttnpb.HasAnyField([]string{"session.keys.s_nwk_s_int_key"}, sets...) && req.EndDevice.Session.SessionKeys.SNwkSIntKey != nil {
+				return nil, nil, errInvalidFieldValue.WithAttributes("field", "session.keys.s_nwk_s_int_key")
 			}
 			// TODO: Encrypt (https://github.com/TheThingsIndustries/lorawan-stack/issues/1562)
 			req.EndDevice.Session.SNwkSIntKey = req.EndDevice.Session.FNwkSIntKey
diff --git a/pkg/networkserver/grpc_deviceregistry_test.go b/pkg/networkserver/grpc_deviceregistry_test.go
index 3da9cf614..4524f4ef7 100644
--- a/pkg/networkserver/grpc_deviceregistry_test.go
+++ b/pkg/networkserver/grpc_deviceregistry_test.go
@@ -464,6 +464,7 @@ func TestDeviceRegistrySet(t *testing.T) {
 					"lorawan_version",
 					"mac_settings.supports_32_bit_f_cnt",
 					"mac_settings.use_adr",
+					"session",
 					"session.dev_addr",
 					"session.keys.f_nwk_s_int_key.key",
 					"session.last_f_cnt_up",
@@ -653,6 +654,7 @@ func TestDeviceRegistrySet(t *testing.T) {
 					"lorawan_version",
 					"mac_settings.supports_32_bit_f_cnt",
 					"mac_settings.use_adr",
+					"session",
 					"session.dev_addr",
 					"session.keys.f_nwk_s_int_key.key",
 					"session.last_f_cnt_up",
@@ -832,6 +834,7 @@ func TestDeviceRegistrySet(t *testing.T) {
 					"lorawan_version",
 					"mac_settings.supports_32_bit_f_cnt",
 					"mac_settings.use_adr",
+					"session",
 					"session.dev_addr",
 					"session.keys.f_nwk_s_int_key.key",
 					"session.last_f_cnt_up",

rvolosatovs

The new NS changes look good

johanstokking added c/network server This is related to the Network Server ui/cli This is related to ttn-lw-cli compat/api This could affect API compatibility labels Sep 26, 2019

johanstokking added this to the September 2019 milestone Sep 26, 2019

johanstokking requested review from htdvisser and rvolosatovs September 26, 2019 20:49

johanstokking self-assigned this Sep 26, 2019

johanstokking mentioned this pull request Sep 26, 2019

Cluster and Service discovery #138

Closed

rvolosatovs suggested changes Sep 29, 2019

View reviewed changes

johanstokking force-pushed the feature/end-device-claiming branch from 71c4366 to 0fbd63f Compare September 29, 2019 15:08

johanstokking requested a review from rvolosatovs September 29, 2019 15:08

johanstokking force-pushed the feature/end-device-claiming branch 2 times, most recently from 409b59c to cc818b5 Compare September 30, 2019 08:21

johanstokking marked this pull request as ready for review September 30, 2019 08:24

johanstokking requested a review from adriansmares September 30, 2019 10:42

johanstokking modified the milestones: September 2019, October 2019 Sep 30, 2019

htdvisser reviewed Oct 1, 2019

View reviewed changes

cmd/ttn-lw-cli/commands/applications_claim.go Outdated Show resolved Hide resolved

johanstokking mentioned this pull request Oct 1, 2019

Show end device QR code #1407

Closed

johanstokking force-pushed the feature/end-device-claiming branch from 55ed248 to 2f758a3 Compare October 1, 2019 20:28

johanstokking requested a review from htdvisser October 1, 2019 20:28

htdvisser approved these changes Oct 2, 2019

View reviewed changes

johanstokking mentioned this pull request Oct 2, 2019

QR code generation service #1410

Closed

johanstokking added the blocked This can't continue until another issue or pull request is done label Oct 2, 2019

johanstokking force-pushed the feature/end-device-claiming branch 2 times, most recently from f868ecc to 03d2b58 Compare October 2, 2019 12:14

johanstokking added the prio/medium label Oct 3, 2019

johanstokking removed the blocked This can't continue until another issue or pull request is done label Oct 3, 2019

johanstokking added 13 commits October 4, 2019 11:50

cli: Add command to claim end devices

a0d1240

cli: Consistent casing in log warnings

b1890a9

all: Interpret TR005 QR code DevVCode as string

b3b7a52

cli: Add application claim (un)authorize commands

b366920

api: Fix validation for QR code

a272555

all: Add RPC middleware to fallback to default port

5bb350a

ns: Remove nil-checks on specified field masks

5b68102

api: Change authentication code value to string

1df69bd

util: Validate QR TR005 draft 2 DevVCode

767a438

util: Support escaped percentage sign

3db3a85

util: Support encoding end device data in QR codes

410c0ef

cli: Add generating QR codes for end devices

fd49fde

all: Regenerate messages

6f89ce3

johanstokking force-pushed the feature/end-device-claiming branch from 03d2b58 to 6f89ce3 Compare October 4, 2019 10:05

rvolosatovs approved these changes Oct 4, 2019

View reviewed changes

adriansmares approved these changes Oct 4, 2019

View reviewed changes

johanstokking merged commit 131cb27 into master Oct 7, 2019

johanstokking deleted the feature/end-device-claiming branch October 7, 2019 09:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Device claiming client #1392

Device claiming client #1392

johanstokking commented Sep 26, 2019 •

edited

coveralls commented Sep 26, 2019 •

edited

rvolosatovs Sep 29, 2019

johanstokking Sep 29, 2019

rvolosatovs Sep 30, 2019

johanstokking Sep 30, 2019

rvolosatovs Oct 3, 2019

johanstokking Oct 3, 2019

rvolosatovs Oct 3, 2019

johanstokking Oct 3, 2019

rvolosatovs Sep 29, 2019

johanstokking Sep 29, 2019

rvolosatovs Sep 29, 2019

johanstokking Sep 29, 2019

johanstokking commented Oct 2, 2019

rvolosatovs commented Oct 3, 2019 •

edited by johanstokking

rvolosatovs left a comment

Device claiming client #1392

Device claiming client #1392

Conversation

johanstokking commented Sep 26, 2019 • edited

Summary

Changes

Notes for Reviewers

Release Notes

coveralls commented Sep 26, 2019 • edited

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

johanstokking commented Oct 2, 2019

rvolosatovs commented Oct 3, 2019 • edited by johanstokking

rvolosatovs left a comment

Choose a reason for hiding this comment

johanstokking commented Sep 26, 2019 •

edited

coveralls commented Sep 26, 2019 •

edited

rvolosatovs commented Oct 3, 2019 •

edited by johanstokking