TrueCloudLab/frostfs-node
20
1
Fork 27
Code Issues 100 Pull requests 4 Projects Releases 1 Activity Actions

Validate token issuer #528

Merged
fyrchik merged 1 commit from dstepanov-yadro/frostfs-node:fix/check_session_issuer into master 2024-09-04 19:51:01 +00:00
Conversation 6 Commits 1 Files changed 5 +95 -120
dstepanov-yadro commented 2023-07-17 13:50:47 +00:00
Member
Copy link

Add token issuer against object owner validation.

Closes TrueCloudLab/frostfs-sdk-go#117

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
Owner
Copy link

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
Author
Member
Copy link

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
Owner
Copy link

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.
aarifullin approved these changes 2023-07-20 10:18:36 +00:00
alexvanin commented 2023-07-24 11:59:18 +00:00
Owner
Copy link

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 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

> > 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
Author
Member
Copy link

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 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.

> > > 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
Owner
Copy link

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
Owner
Copy link

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
Author
Member
Copy link

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 } ```
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
Owner
Copy link

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
Author
Member
Copy link

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
fyrchik approved these changes 2023-08-07 08:46:10 +00:00
dkirillov commented 2023-08-07 08:57:31 +00:00
Member
Copy link

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
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
Member
Copy link

WithLockSource -> WithInnerRing

WithLockSource -> WithInnerRing
dstepanov-yadro commented 2023-08-29 07:14:29 +00:00
Author
Member
Copy link

fixed

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
Member
Copy link

WithLockSource -> WithNetmapSource

WithLockSource -> WithNetmapSource
dstepanov-yadro commented 2023-08-29 07:14:35 +00:00
Author
Member
Copy link

fixed

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
Member
Copy link

WithLockSource -> WithContainersSource

WithLockSource -> WithContainersSource
dstepanov-yadro commented 2023-08-29 07:14:45 +00:00
Author
Member
Copy link

fixed

fixed
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)
fyrchik commented 2023-08-29 07:41:33 +00:00
Owner
Copy link

Are we looking at the object or request signature here?

Are we looking at the object or request signature here?
fyrchik approved these changes 2023-08-29 07:47:34 +00:00
fyrchik merged commit 55b82e744b into master 2023-08-29 07:47:45 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
alexvanin
dkirillov
acid-ant
aarifullin
fyrchik
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
badger

Badger DB related issues

  
frostfs-adm

   
frostfs-cli

   
frostfs-ir

   
frostfs-lens

   
frostfs-node

   
good first issue

Good for newcomers

  
triage
  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff

  
blocked

The issue or PR is blocked by something

  
bug

Something is not working

  
config

Configuration format update or breaking change

  
discussion

Talking about something in order to reach a decision or to exchange ideas

  
documentation

Documentation related

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
go

Language features adoption

  
help wanted

Extra attention is needed

  
internal

   
invalid

Something is wrong

  
kludge

This is a kludge and we know it

  
observability

Related to metrics, tracing and logs

  
perfomance

Perfomance related

  
question

More information is needed

  
refactoring

Refactoring

  
wontfix

This won't be fixed

No labels
P0
P1
P2
P3
badger
frostfs-adm
frostfs-cli
frostfs-ir
frostfs-lens
frostfs-node
good first issue
triage
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
6 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Depends on
#175 Use gate key in owner ID field of the produced objects
TrueCloudLab/frostfs-s3-gw
Reference: TrueCloudLab/frostfs-node#528
No description provided.
Delete branch "dstepanov-yadro/frostfs-node:fix/check_session_issuer"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?