Skip to content

Comments

Access session keys by account only#3817

Open
End-rey wants to merge 2 commits intomasterfrom
3793-access-session-keys-by-account-only
Open

Access session keys by account only#3817
End-rey wants to merge 2 commits intomasterfrom
3793-access-session-keys-by-account-only

Conversation

@End-rey
Copy link
Contributor

@End-rey End-rey commented Feb 12, 2026

Closes #3793.

@End-rey End-rey self-assigned this Feb 12, 2026
@End-rey End-rey added the blocked Can't be done because of something label Feb 12, 2026
@End-rey
Copy link
Contributor Author

End-rey commented Feb 12, 2026

Blocked by #3785.

@End-rey End-rey force-pushed the 3793-access-session-keys-by-account-only branch from 2a3b793 to 6aa3143 Compare February 16, 2026 19:39
}
}

privKey, err := x509.ParseECPrivateKey(rawKey)
Copy link
Member

Choose a reason for hiding this comment

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

Wow. We really use DER here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's the problem with them?

Copy link
Member

Choose a reason for hiding this comment

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

Not that it's a big problem, but not something I'd expect either. NeoGo never does it, for example. To be fair, the most appropriate storage format here would probably be NEP-2, but that can be done in #3426.

Copy link
Member

Choose a reason for hiding this comment

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

can be migrated too if it is a problem

zap.String("token_id", hex.EncodeToString(k)),
)
}
// iterating over accounts
Copy link
Member

Choose a reason for hiding this comment

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

Iterating over all of them is a bad idea. Rather there should be an index like {epoch}{user.ID}, but that's a different issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there already such an issue? Or should I create one?

Copy link
Member

Choose a reason for hiding this comment

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

Please add an issue for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking, how should we do GetToken then? It occurred to me that we could either have a separate bucket for such an index, in which case we would have two buckets, or GetToken would search by cursor, which removes O(1) token retrieval.

Copy link
Member

Choose a reason for hiding this comment

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

You're storing {acc} -> {key} now, it can be {pref_0}{acc} -> {key}, then a separate {pref_1}{epoch}{acc} key without any value. Adding a key is adding two keys into the same bucket. Then checking for obsolete values is trivial, Seek() to {pref_1} and grab items till epoch is not current. Then drop all of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I understand. I overthought it, just need another key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@roman-khimov roman-khimov removed the blocked Can't be done because of something label Feb 20, 2026
@End-rey End-rey force-pushed the 3793-access-session-keys-by-account-only branch from 6aa3143 to 175c665 Compare February 20, 2026 14:54
@codecov
Copy link

codecov bot commented Feb 20, 2026

Codecov Report

❌ Patch coverage is 43.79085% with 86 lines in your changes missing coverage. Please review.
✅ Project coverage is 25.62%. Comparing base (5ab98cc) to head (18553f8).

Files with missing lines Patch % Lines
pkg/util/state/migratation.go 48.88% 31 Missing and 15 partials ⚠️
pkg/services/object/delete/delete.go 0.00% 5 Missing ⚠️
pkg/services/object/get/exec.go 0.00% 5 Missing ⚠️
pkg/services/object/search/util.go 0.00% 5 Missing ⚠️
pkg/services/object/server.go 0.00% 5 Missing ⚠️
pkg/util/state/token.go 68.75% 4 Missing and 1 partial ⚠️
cmd/neofs-node/object.go 0.00% 4 Missing ⚠️
cmd/neofs-node/config.go 0.00% 2 Missing ⚠️
pkg/services/object/put/remote.go 60.00% 1 Missing and 1 partial ⚠️
pkg/services/object/put/streamer.go 60.00% 1 Missing and 1 partial ⚠️
... and 3 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3817      +/-   ##
==========================================
+ Coverage   25.58%   25.62%   +0.03%     
==========================================
  Files         667      667              
  Lines       42941    43011      +70     
==========================================
+ Hits        10988    11021      +33     
- Misses      30949    30974      +25     
- Partials     1004     1016      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@End-rey End-rey requested a review from roman-khimov February 20, 2026 15:07
CHANGELOG.md Outdated
### Changed
- SN retries notary requests if `insufficient amount of gas` error appears (#3739)
- Speed up metabase resync by using batch operations (#3804)
- Session key storage access session keys by account only (#3817)
Copy link
Member

Choose a reason for hiding this comment

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

Wrong section

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

}
}

p.l.Info("duality token migration completed",
Copy link
Member

Choose a reason for hiding this comment

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

session token storage migration completed

And this can logged outside of transaction, btw.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


owners := make(map[user.ID]int)
c := rootBucket.Cursor()
for ownerKeyBytes, v := c.First(); ownerKeyBytes != nil; ownerKeyBytes, v = c.Next() {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this will be performed on every node start, while in fact it's needed only once. Can this be solved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If data has migrated, then their v != nil, meaning that continue will be called below, on the first check. Or should we avoid the for loop as well?

Copy link
Member

Choose a reason for hiding this comment

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

Ideally you should just read something (no DB changes at all) and move on if there is nothing to do.

Copy link
Member

Choose a reason for hiding this comment

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

Because Update() itself will write to the DB even if there are no changes done. Also we'd get a migration message when no migration was done in fact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we assume that migration is performed only once and only for the old version of token storage, then before starting migration, we can first View the first key in the bucket, and if it has a value, which means that it is not a nested bucket, then the data has already been migrated. Is this option acceptable?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, should be ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

}
}

privKey, err := x509.ParseECPrivateKey(rawKey)
Copy link
Member

Choose a reason for hiding this comment

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

Not that it's a big problem, but not something I'd expect either. NeoGo never does it, for example. To be fair, the most appropriate storage format here would probably be NEP-2, but that can be done in #3426.

zap.String("token_id", hex.EncodeToString(k)),
)
}
// iterating over accounts
Copy link
Member

Choose a reason for hiding this comment

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

Please add an issue for it.

fatalOnErr(err)
}

err = persistate.MigrateSessionTokensToAccounts()
Copy link
Member

Choose a reason for hiding this comment

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

it gonna migrate until we remove it? can be tight to node's version. but no-op migration should not be a problem though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we'll remove it at some point, but I don't know how to determine when that point will be.

}
privKey, err := p.extractKeyFromPackedToken(tokenData)
if err != nil {
if p.l != nil {
Copy link
Member

Choose a reason for hiding this comment

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

do we expect it to be nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, if dropped.

}
}

privKey, err := x509.ParseECPrivateKey(rawKey)
Copy link
Member

Choose a reason for hiding this comment

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

can be migrated too if it is a problem

@End-rey End-rey force-pushed the 3793-access-session-keys-by-account-only branch 2 times, most recently from ea12b52 to 18553f8 Compare February 20, 2026 17:06
Contains obtaining auth key from session token.
Also contains fix #3805.

Signed-off-by: Andrey Butusov <andrey@nspcc.io>
There is no need to store session information in two formats in session key
storage. An ID is not required, as it can be retrieved using the public key
account of the open session.

Closes #3793.

Signed-off-by: Andrey Butusov <andrey@nspcc.io>
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.

Access session keys by account only

3 participants