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.

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

Adds ake.Server Serde methods #8

Merged
merged 13 commits into from
Jul 23, 2021
Merged

Conversation

everyCTO
Copy link
Contributor

@everyCTO everyCTO commented Jul 9, 2021

This is an implementation of the request in #7. Please refer to that discussion for background.

Changes in tests/opaque_test.go are from the discussion and are not meant to make any material change to the test.

The focus of this PR is adding two new methods to ake.Server:

// SerializeState will return a []byte with the given capacity containing
// internal state of the Server.
func (s *Server) SerializeState(size int) []byte
// DeserializeState will set internal state onto the server. `size` should the
// output of internal.Parameters.MAC.Size()
func (s *Server) DeserializeState(data []byte, size int) error

Example usage from the modified test:

// serialize the state to be persisted elsewhere
state = server.Ake.SerializeState(server.MAC.Size())
// retrieve serialized state and deserialize onto server
err := server.Ake.DeserializeState(state, server.MAC.Size())

@bytemare bytemare self-requested a review July 22, 2021 23:37
internal/ake/server.go Outdated Show resolved Hide resolved
internal/ake/server.go Outdated Show resolved Hide resolved
internal/ake/server.go Outdated Show resolved Hide resolved
internal/ake/server.go Outdated Show resolved Hide resolved
internal/ake/server.go Outdated Show resolved Hide resolved
@bytemare
Copy link
Owner

bytemare commented Jul 23, 2021

Thank you ! Nice addition, indeed. I also like the formatting in the test function, it's much better.

I left some comments and suggestions. Do you think you can address them or would you prefer me to do it?

Also, I pushed some changes that might break some things, I fear that a rebase from the main branch is necessary.

@everyCTO
Copy link
Contributor Author

Thank you ! Nice addition, indeed. I also like the formatting in the test function, it's much better.

😄

I left some comments and suggestions. Do you think you can address them or would you prefer me to do it?

I'd be happy to address them.

Also, I pushed some changes that might break some things, I fear that a rebase from the main branch is necessary.

No problem. I'll rebase.

@everyCTO
Copy link
Contributor Author

rebased and pushed. I'll incorporate the suggested changes and push again.

@bytemare
Copy link
Owner

I just pushed some changes that should simplify the configuration and its usage a bit, but I don't think it impacts your changes

`internal/ake/server.go`:
- remove import of `github.com/bytemare/cryptotools/utils`.
- `ake.Server.SerializeState` no longer takes a size parameter.
- drop `ake.Server.DeserializeState(data []byte, size int)` in favor of
  `ake.Server.SetState(clientMac, sessionSecret []byte) error`.
- add `ake.Server.ExpectedMAC` accessor.

`server.go`:
- add `Server.SerializeState() []byte` which delegates to
  `ake.Server.SerializeState()`
- add `Server.DeserializeAKEState(state []byte) error` to set AKE server
  state using `ake.Server.SetState`
- add `Server.ExpectedMAC` which delegates to the
  `ake.Server.ExpectedMAC` accessor.
- update comments to more accurately reflect when AKE server state is
  set.

`tests/opaque_test.go`:
- update `FullTest` to use `server.SerializeState()` and
  `server.DeserializeAKEState()` directly.
Copy link
Contributor Author

@everyCTO everyCTO left a comment

Choose a reason for hiding this comment

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

Updated to incorporate feedback.

internal/ake/server.go Show resolved Hide resolved
server.go Show resolved Hide resolved
Comment on lines +150 to +153
// ExpectedMAC returns the expected client MAC if the previous call to Init() was successful.
func (s *Server) ExpectedMAC() []byte {
return s.Ake.ExpectedMAC()
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It turns out, for my use case, it's useful to have an accessor for the expected MAC. Without it, I would first need to fetch the server's serialized state from a database, rehydrate it using DeserializeAKEState, call Finish, and then make another call to the database to persist an authenticated session.

I do already have SessionKey, so I could theoretically determine the expected MAC by trimming off SessionKey as a suffix from the output of SerializeState; what's left is the MAC. But that's breaking the abstraction.

By adding this accessor, I can make a single call to the database (in a transaction) to assert that the MAC I have from the client matches the expected MAC, and persist an authenticated session.

server.go Outdated Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
Co-authored-by: Daniel Bourdrez <3641580+bytemare@users.noreply.github.com>
server.go Outdated Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
@bytemare
Copy link
Owner

One more thing, and I think we're good to go !

Can you add relevant tests in failling_test.go?

@bytemare bytemare self-assigned this Jul 23, 2021
@bytemare bytemare self-requested a review July 23, 2021 15:50
@everyCTO
Copy link
Contributor Author

everyCTO commented Jul 23, 2021

I see some linting complaints when I run golangci-lint locally:

$ golangci-lint run --config=./.github/.golangci.yml ./...
WARN [runner] The linter 'scopelint' is deprecated due to: The repository of the linter has been deprecated by the owner. Use 'exportloopref' instead.
WARN [runner] The linter 'maligned' is deprecated due to: The repository of the linter has been archived by the owner. Use govet 'fieldalignment' instead.
internal/ake/server.go:92:18: Comment should end in a period (godot)
// internal state
                 ^
server.go:185:77: Comment should end in a period (godot)
// SetAKEState sets the interal state of the AKE server from the given bytes
                                                                            ^
server.go:195:9: Comment should end in a period (godot)
// bytes
        ^
internal/ake/server.go:100:2: return statements should not be cuddled if block has more than two lines (wsl)
        return nil
        ^
internal/ake/server.go:95:10: err113: do not define dynamic errors, use wrapped static errors instead: "errors.New(\"existing state is not empty\")" (goerr113)
                return errors.New("existing state is not empty")
                       ^
server.go:188:10: err113: do not define dynamic errors, use wrapped static errors instead: "errors.New(\"invalid state length\")" (goerr113)
                return errors.New("invalid state length")
                       ^

Should I address those? some of them are from areas of the code base that are not changing in this PR. Happy to address either way.

server.go Outdated Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
internal/ake/server.go Outdated Show resolved Hide resolved
internal/ake/server.go Outdated Show resolved Hide resolved
internal/ake/server.go Show resolved Hide resolved
@bytemare
Copy link
Owner

I see some linting complaints when I run golangci-lint locally:

...

Should I address those? some of them are from areas of the code base that are not changing in this PR. Happy to address either way.

I excluded some linters from running in the internal packages, as there is a lot of code that is still subject to change, and I don't consider some results as a priority (e.g. variables and functions that are not exported outside OPAQUE don't need to be included in the external documentation).

But if you're motivated to address them, go for it! )

@everyCTO
Copy link
Contributor Author

I excluded some linters from running in the internal packages, as there is a lot of code that is still subject to change, and I don't consider some results as a priority (e.g. variables and functions that are not exported outside OPAQUE don't need to be included in the external documentation).

But if you're motivated to address them, go for it! )

Makes sense. I might circle back to that in a later PR.

For now, it looks like all of the CI checks are happy except for Snyk, which is asking for a refreshed auth token.

@bytemare
Copy link
Owner

Yeah, let's not care about that right now

@bytemare bytemare merged commit 5b53314 into bytemare:main Jul 23, 2021
@bytemare
Copy link
Owner

Thank you for your contribution ❤️

@everyCTO everyCTO deleted the stateless-server branch July 23, 2021 16:27
@bytemare
Copy link
Owner

Hey @everyCTO,

FYI I took some time to add the comments to the internal package. Your linter should now complain a bit less :) I also extended the test you added to include the case of an existing state for the server.

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.

2 participants