Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix unauthorized container ops with sessions #2958

Merged
merged 2 commits into from
Oct 7, 2024
Merged

Fix unauthorized container ops with sessions #2958

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7c6fc5f
ir: Check session token signer against the container owner
cthulhu-rider Oct 2, 2024
4133364
cli: Control issuer redefinition in the session token signing cmd
cthulhu-rider Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ attribute, which is used for container domain name in NNS contracts (#2954)

### Fixed
- Do not search for tombstones when handling their expiration, use local indexes instead (#2929)
- Unathorized container ops accepted by the IR (#2947)

### Changed
- `ObjectService`'s `Put` RPC handler caches up to 10K lists of per-object sorted container nodes (#2901)
Expand Down
19 changes: 18 additions & 1 deletion cmd/neofs-cli/modules/util/sign_session.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,18 @@
"github.com/nspcc-dev/neofs-node/cmd/neofs-cli/internal/common"
"github.com/nspcc-dev/neofs-node/cmd/neofs-cli/internal/commonflags"
"github.com/nspcc-dev/neofs-node/cmd/neofs-cli/internal/key"
neofscrypto "github.com/nspcc-dev/neofs-sdk-go/crypto"
neofsecdsa "github.com/nspcc-dev/neofs-sdk-go/crypto/ecdsa"
"github.com/nspcc-dev/neofs-sdk-go/session"
"github.com/nspcc-dev/neofs-sdk-go/user"
"github.com/spf13/cobra"
)

// signSessionCmd specific flags.
const (
forceIssuerFlag = "force-issuer"
)

var signSessionCmd = &cobra.Command{
Use: "session-token",
Short: "Sign session token to use it in requests",
Expand All @@ -31,6 +38,7 @@
_ = signSessionCmd.MarkFlagRequired(signFromFlag)

flags.String(signToFlag, "", "File to save signed session token (optional)")
flags.Bool(forceIssuerFlag, false, "Set configured account as the session issuer even if it is already specified")
}

func signSessionToken(cmd *cobra.Command, _ []string) error {
Expand All @@ -47,6 +55,7 @@
json.Marshaler
common.BinaryOrJSON
Sign(user.Signer) error
SetSignature(neofscrypto.Signer) error

Check warning on line 58 in cmd/neofs-cli/modules/util/sign_session.go

View check run for this annotation

Codecov / codecov/patch

cmd/neofs-cli/modules/util/sign_session.go#L58 

Added line #L58 was not covered by tests
}
var errLast error
var stok iTokenSession
Expand All @@ -71,7 +80,15 @@
return err
}

err = stok.Sign(user.NewAutoIDSignerRFC6979(*pk))
forceIss, err := cmd.Flags().GetBool(forceIssuerFlag)
if err != nil {
return err

Check warning on line 85 in cmd/neofs-cli/modules/util/sign_session.go

View check run for this annotation

Codecov / codecov/patch

cmd/neofs-cli/modules/util/sign_session.go#L83-L85 

Added lines #L83 - L85 were not covered by tests
}
if forceIss {
err = stok.Sign(user.NewAutoIDSignerRFC6979(*pk))
} else {
err = stok.SetSignature(neofsecdsa.SignerRFC6979(*pk))

Check warning on line 90 in cmd/neofs-cli/modules/util/sign_session.go

View check run for this annotation

Codecov / codecov/patch

cmd/neofs-cli/modules/util/sign_session.go#L87-L90 

Added lines #L87 - L90 were not covered by tests
Copy link
Member

Choose a reason for hiding this comment

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

check already attached issuer? warn/err if does not match/missing?

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 wouldnt add one more key<->user relation. And error is bad here to me cuz this utility should help, not limit. Dont think warning will be useful too: if other issuer is a problem - user will get his error later anyway, if not - there is nothing to worry bout

}
if err != nil {
return fmt.Errorf("can't sign token: %w", err)
}
Expand Down
10 changes: 9 additions & 1 deletion pkg/innerring/processors/container/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,15 @@
return errors.New("invalid session token signature")
}

// FIXME(@cthulhu-rider): #1387 check token is signed by container owner, see neofs-sdk-go#233
var signerPub neofsecdsa.PublicKeyRFC6979
if err = signerPub.Decode(tok.IssuerPublicKeyBytes()); err != nil {
return fmt.Errorf("invalid issuer public key: %w", err)

Check warning on line 75 in pkg/innerring/processors/container/common.go

View check run for this annotation

Codecov / codecov/patch

pkg/innerring/processors/container/common.go#L73-L75 

Added lines #L73 - L75 were not covered by tests
}

// TODO(@cthulhu-rider): #1387 check bound keys via NeoFSID contract?
if user.NewFromECDSAPublicKey(ecdsa.PublicKey(signerPub)) != v.ownerContainer {
return errors.New("session token is not signed by the container owner")

Check warning on line 80 in pkg/innerring/processors/container/common.go

View check run for this annotation

Codecov / codecov/patch

pkg/innerring/processors/container/common.go#L79-L80 

Added lines #L79 - L80 were not covered by tests
}

if keyProvided && !tok.AssertAuthKey(&key) {
return errors.New("signed with a non-session key")
Expand Down
Loading