Add a skynet-utils binary that does trustless portal uploads and downloads #10
Conversation
DavidVorick
changed the title
|
I ended up doing a lot more. The end result is going to be a fully trustless skynet-kernel deployment |
|
I migrated a cli tool over. It's not building, also the Makefile is pretty nonstandard. I might do some re-organization to get the Makefile working. |
DavidVorick
changed the title
DavidVorick
changed the title
| @@ -1,2 +1,3 @@ | |||
| *.swp | |||
There was a problem hiding this comment.
nit
# Ignore temporary vim files
*~
*.swp
*.swo
| @@ -88,9 +88,5 @@ func (sc *SkynetClient) executeRequest(config requestOptions) (*http.Response, e | |||
| if err != nil { | |||
| return nil, errors.AddContext(err, "could not execute request") | |||
| } | |||
| if resp.StatusCode >= 400 { | |||
There was a problem hiding this comment.
do we have a compatibility promise on this repo?
| switch args[1] { | ||
| case "generate-v2skylink", "p", "v2": | ||
| generateV2SkylinkFromSeed(args[2], args[3:]) | ||
| case "upload-to-v2skylink", "u2", "u2v2", "utv", "utv2": |
There was a problem hiding this comment.
I believe this command is missing in printHelp
|
|
||
| // generateV2SkylinkFromSeed will generate a V2 skylink from a seed using the | ||
| // specified salt. | ||
| func generateV2SkylinkFromSeed(salt string, phraseWords []string) { |
There was a problem hiding this comment.
there's no input validation at all, shouldn't we add the minimum?
| } | ||
|
|
||
| // generateAndPrintSeedPhrase will generate a new seed and print it. | ||
| func generateAndPrintSeedPhrase() { |
There was a problem hiding this comment.
nit it would've been neat if these commands were prefixed with cmd or something to indicate what they are, that way it's not weird or surprising to find os.Exit and fmt.Println everywhere
| // the file using UploadFileSecure. | ||
| func FileSkylink(path string) (skylinkStr string, err error) { | ||
| // Create a nil SkynetClient to call the function. This won't cause a | ||
| // panic because the funciton aborts early due to the dryRun flag. |
There was a problem hiding this comment.
| // panic because the funciton aborts early due to the dryRun flag. | |
| // panic because the function aborts early due to the dryRun flag. |
| } | ||
|
|
||
| // Parse the response. | ||
| if resp.ContentLength > 10e6 { |
There was a problem hiding this comment.
wouldn't you usually use a limit reader? I believe this header can be overwritten with whatever value the portal wants it to be.
There was a problem hiding this comment.
Agreed. Blindly using ioutil.ReadAll won't be safe because we can't necessarily trust the content length here. I'd wrap the resp in a io.LimitReader first and then use a dec := json.NewDecoder(limitReader) since that's more efficient than ioutil.ReadAll.
| // WARNING: Improper use of this function can cause data loss, and has caused | ||
| // users data loss in the past. The common mistake is to first read from the | ||
| // registry entry, see that nothing is there, and then call OverwriteRegistry | ||
| // with new data. If the initial read fasely returned 404 or was subject to a |
There was a problem hiding this comment.
| // with new data. If the initial read fasely returned 404 or was subject to a | |
| // with new data. If the initial read falsely returned 404 or was subject to a |
| // the update. That eliminates deceptions where the portal pretends to execute | ||
| // the registry update but actually does not. Because the data is signed, there | ||
| // is no opportunity for the portal to place false or bad data on the network. | ||
| func (sc *SkynetClient) OverwriteRegistry(secretKey crypto.SecretKey, dataKey crypto.Hash, data []byte) error { |
There was a problem hiding this comment.
maybe it makes sense to call this OverwriteRegistryUnsafe, I think the warnings are great but at the same time I think it's unfortunate they "only make it to the docstring", so if you fail to check the docstring you might be completely oblivious to the hidden dangers with using this method on the client
| return "", errors.AddContext(err, "unable to fetch filesize") | ||
| } | ||
| if info.Size() > 4e6 { | ||
| return "", errors.New("cannot perform SecureUploadFile with files larger than 4e6 bytes") |
There was a problem hiding this comment.
nit human-readable filesize would be nice
| // implementation. | ||
| curByte := 0 | ||
| curBit := 0 | ||
| for i := 0; i < 13; i++ { |
There was a problem hiding this comment.
we should probably pull out all the hardcoded values in this function.
It would also be nice to have some testing for all those helpers.
|
|
||
| // Determine the checksum of the phrase. This is copied from the | ||
| // skynet-js implementation. | ||
| checksum := sha512.Sum512(seed[:]) |
There was a problem hiding this comment.
This is not the same as sha3.Sum512 right? But I guess this is what we want right?
I think it would be nice to have a test with a hardcoded seed which was created in mysky to make sure the go code is producing the same results.
| } | ||
|
|
||
| // Parse the response. | ||
| if resp.ContentLength > 10e6 { |
There was a problem hiding this comment.
Agreed. Blindly using ioutil.ReadAll won't be safe because we can't necessarily trust the content length here. I'd wrap the resp in a io.LimitReader first and then use a dec := json.NewDecoder(limitReader) since that's more efficient than ioutil.ReadAll.
Added an environment variable SKYNET_PORTAL that a user can configure so that all cli operations automatically use the user's preferred portal. This is part of getting skynet-kernel to a fully trustless place.