Validate token issuer #528

dstepanov-yadro · 2023-07-17T13:50:47Z

dstepanov-yadro commented

2023-07-17 13:50:47 +00:00

Add token issuer against object owner validation.

Add token issuer against object owner validation. Closes https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/issues/117

dstepanov-yadro force-pushed fix/check_session_issuer from dd8761af02 to e832c9154f

2023-07-17 13:52:13 +00:00

Compare

dstepanov-yadro force-pushed fix/check_session_issuer from e832c9154f to ab88cfa039

2023-07-17 13:52:55 +00:00

Compare

dstepanov-yadro requested review from storage-core-committers 2023-07-17 13:56:15 +00:00

dstepanov-yadro requested review from storage-core-developers 2023-07-17 13:56:15 +00:00

dstepanov-yadro requested review from alexvanin 2023-07-17 13:56:19 +00:00

alexvanin commented

2023-07-18 11:38:45 +00:00

We must enable this validation only for objects with new version in the version field of the object. Otherwise replication process may be broken in existing networks.

And node should not accept objects with older versions unless they are sent by container nodes (SYSTEM group during replication), probably.

/cc @fyrchik, @realloc

We must enable this validation only for objects with new version in the version field of the object. Otherwise replication process may be broken in existing networks. And node should not accept objects with older versions unless they are sent by container nodes (SYSTEM group during replication), probably. /cc @fyrchik, @realloc

dstepanov-yadro commented

2023-07-18 12:28:57 +00:00

Poster

We must enable this validation only for objects with new version in the version field of the object. Otherwise replication process may be broken in existing networks.

And node should not accept objects with older versions unless they are sent by container nodes (SYSTEM group during replication), probably.

/cc @fyrchik, @realloc

Please explain.

Replicator uses remoteSender: https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/support/v0.36/pkg/services/replicator/process.go#L57

Remote sender creates remote target with node's private key: https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/support/v0.36/pkg/services/object/put/remote.go#L108

So this condition token == nil will be true, so nothing changes.

> We must enable this validation only for objects with new version in the version field of the object. Otherwise replication process may be broken in existing networks. > > > And node should not accept objects with older versions unless they are sent by container nodes (SYSTEM group during replication), probably. > > /cc @fyrchik, @realloc Please explain. Replicator uses remoteSender: https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/support/v0.36/pkg/services/replicator/process.go#L57 Remote sender creates remote target with node's private key: https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/support/v0.36/pkg/services/object/put/remote.go#L108 So this condition `token == nil` will be true, so nothing changes.

dkirillov approved these changes 2023-07-18 15:47:54 +00:00

alexvanin commented

2023-07-20 09:58:19 +00:00

So this condition token == nil will be true, so nothing changes.

Let me check that. Session token is a part of the object header, not the part of the object request (unlike bearer token), so I am expecting to see token != nil. I might be wrong, though.

> So this condition token == nil will be true, so nothing changes. Let me check that. Session token is a part of the object header, not the part of the object request (unlike bearer token), so I am expecting to see `token != nil`. I might be wrong, though.

alexvanin referenced this pull request from TrueCloudLab/frostfs-s3-gw

2023-07-20 10:04:02 +00:00

Use gate key in owner ID field of the produced objects #175

aarifullin approved these changes 2023-07-20 10:18:36 +00:00

alexvanin commented

2023-07-24 11:59:18 +00:00

So this condition token == nil will be true, so nothing changes.

Let me check that. Session token is a part of the object header, not the part of the object request (unlike bearer token), so I am expecting to see token != nil. I might be wrong, though.

@dstepanov-yadro I got token != nil and token.AssertAuthKey(&key) == true during replication.

It means that stored objects may fail !token.Issuer().Equals(ownerID) check in incoming release and therefore fail replication.

Steps to reproduce:

Add logs in validateSignatureKey()
Start dev-env
Create container with REP 2 SELECT 4 FROM * placement policy
Upload an object "the old way" without signing complete object in the client (I used this code)
See which nodes store an object
docker stop one of those nodes
Produce new epoch
Look in the logs in remaining two nodes, produced by replicator object.Put request.

frostfs-node 5a4054ee

> > So this condition token == nil will be true, so nothing changes. > > Let me check that. Session token is a part of the object header, not the part of the object request (unlike bearer token), so I am expecting to see `token != nil`. I might be wrong, though. @dstepanov-yadro I got `token != nil` and `token.AssertAuthKey(&key) == true` during replication. It means that stored objects may fail `!token.Issuer().Equals(ownerID)` check in incoming release and therefore fail replication. Steps to reproduce: 1) Add logs in `validateSignatureKey()` 2) Start dev-env 3) Create container with `REP 2 SELECT 4 FROM *` placement policy 4) Upload an object "the old way" without signing complete object in the client (I used [this](https://git.frostfs.info/alexvanin/frostfs-sdk-go/src/branch/compare-clients/compare/frostfs_pool_test.go) code) 5) See which nodes store an object 6) `docker stop` one of those nodes 7) Produce new epoch 8) Look in the logs in remaining two nodes, produced by replicator `object.Put` request. frostfs-node 5a4054ee

dstepanov-yadro force-pushed fix/check_session_issuer from ab88cfa039 to 320a769e01

2023-07-24 14:12:49 +00:00

Compare

dstepanov-yadro force-pushed fix/check_session_issuer from 320a769e01 to 348bd68a7a

2023-07-28 07:01:26 +00:00

Compare

dstepanov-yadro commented

2023-07-28 12:48:15 +00:00

Poster

So this condition token == nil will be true, so nothing changes.

Let me check that. Session token is a part of the object header, not the part of the object request (unlike bearer token), so I am expecting to see token != nil. I might be wrong, though.

@dstepanov-yadro I got token != nil and token.AssertAuthKey(&key) == true during replication.

It means that stored objects may fail !token.Issuer().Equals(ownerID) check in incoming release and therefore fail replication.

Steps to reproduce:

Add logs in validateSignatureKey()

Start dev-env

Create container with REP 2 SELECT 4 FROM * placement policy

Upload an object "the old way" without signing complete object in the client (I used this code)

See which nodes store an object

docker stop one of those nodes

Produce new epoch

Look in the logs in remaining two nodes, produced by replicator object.Put request.

frostfs-node 5a4054ee

Added verification that the signer is either an inner ring node or a container node. Also config parameter added to enable/disable this check.

> > > So this condition token == nil will be true, so nothing changes. > > > > Let me check that. Session token is a part of the object header, not the part of the object request (unlike bearer token), so I am expecting to see `token != nil`. I might be wrong, though. > > @dstepanov-yadro I got `token != nil` and `token.AssertAuthKey(&key) == true` during replication. > > It means that stored objects may fail `!token.Issuer().Equals(ownerID)` check in incoming release and therefore fail replication. > > Steps to reproduce: > > 1) Add logs in `validateSignatureKey()` > 2) Start dev-env > 3) Create container with `REP 2 SELECT 4 FROM *` placement policy > 4) Upload an object "the old way" without signing complete object in the client (I used [this](https://git.frostfs.info/alexvanin/frostfs-sdk-go/src/branch/compare-clients/compare/frostfs_pool_test.go) code) > 5) See which nodes store an object > 6) `docker stop` one of those nodes > 7) Produce new epoch > 8) Look in the logs in remaining two nodes, produced by replicator `object.Put` request. > > frostfs-node 5a4054ee Added verification that the signer is either an inner ring node or a container node. Also config parameter added to enable/disable this check.

dstepanov-yadro force-pushed fix/check_session_issuer from 7ea3816469 to fb1c8026f9

2023-07-28 12:49:50 +00:00

Compare

aarifullin approved these changes 2023-08-01 11:25:05 +00:00

alexvanin approved these changes 2023-08-04 06:26:22 +00:00

alexvanin left a comment

LGTM, DNT. See the comment and let's wait for TrueCloudLab/frostfs-s3-gw#175 before merge so we don't break integration.

LGTM, DNT. See the comment and let's wait for https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/issues/175 before merge so we don't break integration.

config/example/node.yaml Outdated

													
				@ -110,6 +110,7 @@ object:

				  put:

				    pool_size_remote: 100  # number of async workers for remote PUT operations

				    pool_size_local: 200  # number of async workers for local PUT operations

				    skip_session_token_issuer_verification: true # session token issuer verification will be skipped if true

alexvanin commented

2023-08-04 06:25:18 +00:00

Are we going to enable it in the current active prod environments?

I thought more of different behavior for this flag:

when disabled, check token issuer in all PUT requests (for fresh installations with latest version of node and gateway)
when enabled, check only if request was sent by non SYSTEM group (for running environments to keep compatibility).

/cc @fyrchik

Are we going to enable it in the current active prod environments? I thought more of different behavior for this flag: - when disabled, check token issuer in all PUT requests (for fresh installations with latest version of node and gateway) - when enabled, check only if request was sent by non `SYSTEM` group (for running environments to keep compatibility). /cc @fyrchik

dstepanov-yadro commented

2023-08-07 08:48:48 +00:00

Poster

It's already done:

	if v.verifyTokenIssuer {
		signerIsIROrContainerNode, err := v.isIROrContainerNode(obj, binKey)
		if err != nil {
			return err
		}

		if signerIsIROrContainerNode {
			return nil
		}

It's already done: ``` if v.verifyTokenIssuer { signerIsIROrContainerNode, err := v.isIROrContainerNode(obj, binKey) if err != nil { return err } if signerIsIROrContainerNode { return nil } ```

alexvanin added a new dependency 2023-08-04 06:27:17 +00:00

TrueCloudLab/frostfs-s3-gw#175 - Use gate key in owner ID field of the produced objects

fyrchik reviewed 2023-08-04 09:43:43 +00:00

pkg/core/object/fmt.go Outdated

													
				@ -161,3 +192,4 @@

					return nil

				}

				func (v *FormatValidator) isIROrContainerNode(obj *objectSDK.Object, signerKey []byte) (bool, error) {

fyrchik commented

2023-08-04 09:43:42 +00:00

We already have role calculation in the acl service: https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/services/object/acl/v2/classifier.go#L27
Can we reuse it somehow?

We already have role calculation in the acl service: https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/services/object/acl/v2/classifier.go#L27 Can we reuse it somehow?

dstepanov-yadro commented

2023-08-07 06:58:08 +00:00

Poster

Done: sender_classifier moved to object/core package, ACL service and format validator now using it.

Done: `sender_classifier` moved to `object/core` package, ACL service and format validator now using it.

fyrchik marked this conversation as resolved

dstepanov-yadro force-pushed fix/check_session_issuer from fb1c8026f9 to d8b00eec92

2023-08-06 12:38:08 +00:00

Compare

dkirillov referenced this issue from a commit

2023-08-07 08:32:30 +00:00

[#175] Use gate owner as object owner

dkirillov referenced this issue from a commit

2023-08-07 08:37:27 +00:00

[#175] Use gate owner as object owner

dkirillov referenced this pull request from TrueCloudLab/frostfs-s3-gw

2023-08-07 08:38:19 +00:00

[#175] Use gate owner as object owner #181

dkirillov referenced this issue from a commit

2023-08-07 08:43:52 +00:00

[#175] Use gate owner as object owner

fyrchik approved these changes 2023-08-07 08:46:10 +00:00

~~dkirillov referenced this pull request from TrueCloudLab/frostfs-s3-gw 2023-08-07 08:52:26 +00:00~~

[#175] Use gate owner as object owner #181

dkirillov commented

2023-08-07 08:57:31 +00:00

LGTM, DNT. See the comment and let's wait for TrueCloudLab/frostfs-s3-gw#175 before merge so we don't break integration.

Actually we can safely merge this PR before PRs in gate because the problem occurs only when client cut being used. Currently gates don't use this feature.

> LGTM, DNT. See the comment and let's wait for https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/issues/175 before merge so we don't break integration. Actually we can safely merge this PR before PRs in gate because the problem occurs only when client cut being used. Currently gates don't use this feature.

dkirillov approved these changes 2023-08-07 08:57:58 +00:00

dkirillov referenced this issue from a commit

2023-08-07 09:09:20 +00:00

[#66] Use gate key to form object owner

dkirillov referenced this pull request from TrueCloudLab/frostfs-http-gw

2023-08-07 09:10:13 +00:00

[#66] Use gate key to form object owner #68

acid-ant approved these changes 2023-08-08 07:33:16 +00:00

pkg/core/object/fmt.go Outdated

													
				@ -383,2 +433,4 @@

					}

				}

				// WithLockSource return option to set Inner Ring source.

acid-ant commented

2023-08-08 07:29:35 +00:00

WithLockSource -> WithInnerRing

dstepanov-yadro commented

2023-08-29 07:14:29 +00:00

Poster

fixed

pkg/core/object/fmt.go Outdated

													
				@ -385,0 +440,4 @@

					}

				}

				// WithLockSource return option to set Netmap source.

acid-ant commented

2023-08-08 07:30:08 +00:00

WithLockSource -> WithNetmapSource

dstepanov-yadro commented

2023-08-29 07:14:35 +00:00

Poster

fixed

pkg/core/object/fmt.go Outdated

													
				@ -385,0 +447,4 @@

					}

				}

				// WithLockSource return option to set Containers source.

acid-ant commented

2023-08-08 07:30:28 +00:00

WithLockSource -> WithContainersSource

dstepanov-yadro commented

2023-08-29 07:14:45 +00:00

Poster

fixed

dkirillov referenced this issue from a commit

2023-08-08 09:22:54 +00:00

[#175] Use gate owner as object owner

aarifullin approved these changes 2023-08-21 09:33:11 +00:00

dstepanov-yadro force-pushed fix/check_session_issuer from fea83d546b to 515dc1eece

2023-08-29 07:10:36 +00:00

Compare

dstepanov-yadro force-pushed fix/check_session_issuer from 515dc1eece to 03d446b637

2023-08-29 07:14:24 +00:00

Compare

dstepanov-yadro force-pushed fix/check_session_issuer from 03d446b637 to 5793f5fab7

2023-08-29 07:23:49 +00:00

Compare

dstepanov-yadro force-pushed fix/check_session_issuer from 5793f5fab7 to 55b82e744b

2023-08-29 07:33:18 +00:00

Compare

fyrchik reviewed 2023-08-29 07:41:33 +00:00

pkg/core/object/fmt.go

													
				@ -159,0 +172,4 @@

					}

					if v.verifyTokenIssuer {

						signerIsIROrContainerNode, err := v.isIROrContainerNode(obj, binKey)