-
Notifications
You must be signed in to change notification settings - Fork 267
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
wasi: Implement random_get
function
#237
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! In general we must not have WASI concepts into wasm package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've done a great job. Once the randomizer is re-organized (and probably only exposed for testing), it will be a quick final review!
ps we added the DCO today, something we forgot to add before, so that's why your PR is red |
wasi/wasi.go
Outdated
@@ -162,6 +179,7 @@ type api struct { | |||
opened map[uint32]fileEntry | |||
// timeNowUnixNano is mutable for testing | |||
timeNowUnixNano func() uint64 | |||
randSource *rand.Rand |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine one would expect this to be "cryptographically secure random number generator" rather than the pseudo one (See getrandom(2) on linux https://man7.org/linux/man-pages/man2/getrandom.2.html, and wasi's get_random is the counter part for that).
How about using crypto/rand instead of math.Rand?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can, for sure
WASI spec says "high-quality" random values, and it's somewhat confusing what they mean under this.
Will move now to crypto/rand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I agree crypto is better as many things in WASI promise things of higher security, it fits
wasi/wasi.go
Outdated
@@ -354,6 +374,10 @@ func newAPI(opts ...Option) *api { | |||
return ret | |||
} | |||
|
|||
func (a *api) seedRandSource(seed int64) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need this here as this is only for tests
// * buf - is a offset to write random values | ||
// * bufLen - size of random data in bytes | ||
// | ||
// For example, if `HostFunctionCallContext.Randomizer` initialized |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this drifted and also we are probably being too specific on the mechanics of random.
maybe For example, if bufLen = 5, we expect
ctx.Memory.Buffer to contain 5 random bytes:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codefromthecrypt
You mean not to use in comments concrete random bytes from specified seed?
And also not to check concrete random bytes form seed, cause crypto/rand non-deterministic and we can't seed it, right?
Just check in test that memory is filled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no I mean in this godoc we don't need to get into how secure random works. I still like that you are using tests that are resulting in a known value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or we can seed math/rand from crypto/rand, like this
var seed int64
binary.Read(crypto_rand.Reader, binary.BigEndian, &seed)
math_rand.NewSource(seed)
and in tests still use simple seed and check concrete bytes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no I mean in this godoc we don't need to get into how secure random works. I still like that you are using tests that are resulting in a known value
got it, thanks for your feedback
Signed-off-by: r8d8 <ckryvomaz@gmail.com>
This adds a link to status we didn't have (thanks to @mbovel) Signed-off-by: Adrian Cole <adrian@tetrate.io> Signed-off-by: r8d8 <ckryvomaz@gmail.com>
Signed-off-by: r8d8 <ckryvomaz@gmail.com>
Signed-off-by: r8d8 <ckryvomaz@gmail.com>
…s#231) This consolidates more code around SectionID, notably consistently getting and using section ID length. This also enforces rules in text parsing such as up to one memory, and imported functions can't be defined after regular ones. Signed-off-by: Adrian Cole <adrian@tetrate.io> Signed-off-by: r8d8 <ckryvomaz@gmail.com>
Per tetratelabs#232, we will be unexporting fields only used during instantiate. Any additional custom sections are unusable otherwise, so this skips them. In doing so, this eliminates a length check bug, some TODO and some logic that enforced strict name uniqueness even though that only rule only applied to the name section. Note: this doesn't restrict future use of custom sections, ex for DWARF, just we don't buffer sections we don't use anymore. See https://www.w3.org/TR/wasm-core-1/#custom-section%E2%91%A0 Signed-off-by: Adrian Cole <adrian@tetrate.io> Signed-off-by: r8d8 <ckryvomaz@gmail.com>
Signed-off-by: r8d8 <ckryvomaz@gmail.com>
Signed-off-by: r8d8 <ckryvomaz@gmail.com>
Signed-off-by: r8d8 <ckryvomaz@gmail.com>
* Remove random generator from WASM structure. * Adjust formatting and naming. Signed-off-by: r8d8 <ckryvomaz@gmail.com>
Use pseudo-random source in tests. Signed-off-by: r8d8 <ckryvomaz@gmail.com>
Signed-off-by: Adrian Cole <adrian@tetrate.io> Signed-off-by: r8d8 <ckryvomaz@gmail.com>
Signed-off-by: Adrian Cole <adrian@tetrate.io> Signed-off-by: r8d8 <ckryvomaz@gmail.com>
ccd4f3f
to
a69be1a
Compare
Will cancel this PR and open new one |
No description provided.