The user-verifying code in path.Parse is too weak and leads to a bad code path. This means that p=path.Parse("[email protected]*") does a bad job, not putting a slash in the name, and p.User() will then crash because there is no slash in the supposedly canonicalized string.

Diagnose and fix.

The test for List permission needs to be done in the loop over "this". As written, a pattern like a/STAR/b/STAR will cause the code to call access.Can("a/STAR/b"), which is nonsense. (Here STAR means asterisk, which github won't let me show.)

Also, it needs to initialize itself based on the value in the logs package, or at least coordinate with it.

While a pending change will make it harder for clients to connect to servers that use self-signed certificates, we should provide a means for users to specify the specific self-signed certs that they do trust.

This way, a user could set up a private upspin cluster somewhere without CA-signed certs, but still have some degree of security. It would also make our testing easier.

cc @presotto @n2vi @edpin @robpike

The DirServer will be doing all the parsing and high-level validations. Parsing everything again is wasteful.

The fileserver binary is read-only for simplicity, but it could allow writes by staging Store.Put data in a temporary directory until the corresponding Directory.Put arrives. This would require maintaining a mapping from reference to file name.

It's of type []byte, which makes it a little easier to work with, but it should nonetheless be the type for the purpose.

I have memories of this type changing before. Whatever the history, it should be Packdata.

The Location type that it currently returns serves no purpose. I suggest returning either nothing (only an error) or perhaps the DirEntry describing it.

What character set? What is illegal? Is space allowed? Valid UTF-8 or arbitrary bytes? etc. etc.

This is just missing from the code and our model: A WhichAccess call should require that the caller has List permission for the path. We might also require more, such as List or even Read for the Access file itself. Not sure.

Discuss.

from edpin:

Client should cache AuthToken on stable storage to avoid re-authenticating (cmd/upspin will Auth every time otherwise). Server should remember the old token and return that to the user or invalidate it, to avoid polluting the cache with new tokens (only one valid token should be around for each user). (How would the same user on two clients share a token? Maybe invalidating by default it not a good idea)

No details available; issue taken from original issue doc.

The cmd/fileserver should really just be an upspin.StoreService and upspin.DirService implementation pair, that are served through the existing cmd/storeserver and cmd/dirserver.

Presently, our GRPC clients allow servers to use self-signed TLS certs by default. That's bad.

dir/remote/remote.go:   authClient, err := grpcauth.NewGRPCClient(context, e.NetAddr, grpcauth.KeepAliveInterval, grpcauth.AllowSelfSignedCertificate)
key/remote/remote.go:   authClient, err := grpcauth.NewGRPCClient(context, e.NetAddr, grpcauth.KeepAliveInterval, grpcauth.AllowSelfSignedCertificate)
store/remote/remote.go: authClient, err := grpcauth.NewGRPCClient(context, e.NetAddr, grpcauth.KeepAliveInterval, grpcauth.AllowSelfSignedCertificate)

and

auth/grpcauth/client.go:        // AllowSelfSignedCertificate is used for documenting the parameter with same name in NewGRPCClient.
auth/grpcauth/client.go:        AllowSelfSignedCertificate = true

The directory should be backed by a Merkle Tree, to enable easy and efficient backups.

Missing items:

• Extensive Access/permission checking, including Groups.
• Cross dirserver Group files.
• Sequence number checking.
• Delete root.
• Deletes.
• Links.
• Add permutation to include the caches.
• Snapshots and snapshot configuration.
• Various error conditions such as:
  • malformed paths/usernames
  • check for leaking directory information
  • flaky network/server
  • read-only nature of snapshots
• Watch.

(edit this post to add more)

The current tests are too simple.

There are some simple tests, and some in the integration test, but there are many cases that have not been tested explicitly.

Also, it is important to verify that all services implement them correctly.

On a related note, the Access.Can method has no explicit tests in its test suite, only indirect ones. This might be a good place to start.

It is not needed.

They do at the moment, and PutDuplicate's test checks that. But it's inconsistent and undocumented.

Resolve.

We allow a user to use Group files from another user in his/her Access files. Such Group files could potentially be stored in a different DirServer.

That creates a small issue: the DirServer may need to use a Client for fetching the data for that Group file, since it may not be able to resolve it locally.

Do we want to allow the DirServer to contact other DirServers, possibly using the Client or a stripped-down version of Client? It seems the only use case for this is with Group files. Everything else we managed to push to Clients (links) or resolve locally.

For now, we're disallowing Group files in other DirServers AND from other users*. But we should allow both going forward.
.

• The reason for disallowing other users even in the same DirServer is because in the current DirServer implementation we'd need to hold two locks -- one for each of the two users -- at the same time and there is no ordering that guarantees no deadlock (say, both A and B include each other's Group file and they access something that needs them at the same time). A global lock could resolve that, but it would complicate everything else. If we solve the general problem, this can solved the same way: start a client in a go routine to fetch the Group file with only one lock held). And cache the hell out of it.

For updating signatures and key wrapping after key rotation, I'm going to be needing to walk file trees. This can be a subtle design topic, I vaguely recall from the past, so it would be nice to have it as a utility library function.

The cmd/upspin might make good use of it to add -R functionality to ls and info.

The upspinfs implementation should be fast and reliable.

Forgout about them. Adding next.

Most methods in the Tree return pointers to dir entries, which allow the caller to modify them and hence make them "dirty" without telling the tree. This could be a source of very hard-to-track bugs. We should consider returning copies instead.

However, returning a copy is more costly. And even though the server needs to make another copy over the network, which is orders of magnitude more expensive anyway, not all uses of these functions end with a copy over the network, especially for Lookup and WhichAccess. The DirServer often calls Lookup and WhichAccess internally to check for things like permissions, whether an entry is a directory, a link, etc. So, avoiding multiple copies per API call is a good thing.

This is not a clear case of a black-and-white decision. Measurements would help in determining the impact of the internal copies. We also need to worry about realistic workloads, with large files and large trees.

Currently, there is never a situation where we need to do a lookup and modify the entry in-place. Even when overwriting a file, we do a lookup to a temporary variable and then put the new one.

So, for now, I've documented the interface prohibiting the DirServer from modifying entries directly.

Line 167 (at least at time of writing) reads:

// r.read(user, publicFile, success) TODO: Unpack: could not find wrapped key

This needs to be understood and fixed. The solution might be just to avoid this test for EE pack, but it should be understood first.

By modeling files as a sequence of references, we can make random reads and writes within files more efficient.

Right now, the dirserver will look for the user server hard-coded in auth/config.go:

const userServiceAddr = "upspin.io:5582"

It should instead use the user server from its context, which should be specified in the server's rc file.

When we moved transport Remote to be value 3 instead of 2, the userserver was reporting the wrong transport because the JSON entry contained the value "2", not "remote". If we're going to use JSON or some human-friendly format, we need to be more robust to the precise value used.

They are no longer needed now that we are using GRPC.

I see upper case proto field names, lower case fields and fields with underscores, like so:
Name
UserName
reference
data
user_name

Decide which style we want and fix it.

It's expensive to look up user keys one at a time. It would be easy to make it handle multiple users in a single call for efficiency when doing things like Share.

Access and Group files (AGFs) have restricted properties. They must be plain text and cannot be linked.

We might be able to use this to good advantage. If DirServers held the storage for them directly, it would greatly simplify them accessing their contents.

This would most simply require DirServers to implement the Store interface (there is a name collision with Put but we can sort that out). They could then enforce that AGFs have only local references, and so a DirEntry for an AGF resides on the same computer as the storage for it.

This would mean a DirServer wouldn't need all the mess of a client implementation for cross-server Groups, just the ability to do store.Get.

See also #37.

They were used in a confusing dance to get configuration right for some transports/servers. No longer needed.

edpin writes:

GCP directory doesn’t like Put()s of zero length Access files. We get errors when it reads the Access file from the store, for example: “store error: Get: https://www.googleapis.com/download/storage/v1/b/g-upspin-store/o/E3B0C44298FC1C149AFBF4C8996FB92427AE41E4649B934CA495991B7852B855?generation=1466052594464000&alt=media: EOF”

log is /tmp/upspinfs23268.log
creating good user didn't work but should have
--- FAIL: TestShell (14.39s)
sh_test.go:18: exit status 1
FAIL

The log file is deleted so it's hard to track down what happened.

Rob suggests: "If the test fails, I think the shell script should not delete the log. That would help debugging."

This may happen automatically through the move to containers.

The WhichAccess right checked now is the List right. Instead, it should verify that the caller has any right at all: If you are allowed to know something about the file, you should know the name of the Access file controlling it.

To do this efficiently, provide a way to ask an Access file if any right is available to the user.

When a link is hidden from a user (because its ruling Access file prohibits it from being Listed), but the link links to a path the user has Access, what should happen? ErrFollowLink or errNotExist?

ErrFollowLink it is for now.

Currently the API for turning off application pings is not exposed to Android. However, it doesn't need to be. Android can simply timeout the entire client when the app is not visible (onStop or onPause) or set a timer to do so.

This is important because these frequent pings harm the battery life more than they gain by keeping a connection alive with the server.

In summary: the client needs to be thrown away somewhat quickly when not used.

We need to design the mechanism that enables writing to stores other than your primary store.

For example, if a file system is linked into your root, you should be able to write into that file system and have the references end up in that file system's store (not whatever store you have specified in your rc).

This also means we need to design directory linking.

All binaries will benefit.

We have lazy heuristics for email address validation. Dave P. reports these are the rules and we should obey them. Note that it means user names should be lower-cased in path.Parse and in KeyServer.Put and KeyServer.Lookup.

We could start out with the Facebook and Google rules for user names (except for the + part).

I suggest we ignore punycode for now.

[name] := [user name]@[domain name]

[domain name] :=

• each . separated token < 64 characters
• character set for tokens [a-z0-9-]
• final token at least two characters
• whole name < 254 characters
• characters are case insensitive

There is an IDNA standard currently blessed by the IANA which is the internationalization of punycode, i.e., something that encodes an international set into ascii. We could ignore that for now since it is essentially a client conversion and we can require that they use the converted string. Don't know how much that would piss people off since its a pain to do the mental conversion. This is the definition https://tools.ietf.org/html/rfc3490 .

[user name] :=

• pretty much anything goes. This is not an email name so it is whatever the system they log into will accept. However, almost all are constrained by what smtp will allow since they are almost invariably also used for email. So I would expect a superset of what will make it through mail:
• upper and lower case a-z, case matters.
• 0-9 !#$%&'*+-/=?^_{|}~
• utf
• spaces

So "John [email protected]" would work (double quotes are my way of delineating the name here, not part of the name).

Facebook and google constrain you to a-zA-Z0-9+-. . Period and in Google anything after the + is ignored.

These are the use cases we aspire to cover before the end of 2016.

• Provide a reliable shared Unix file system, accessible to one user from multiple machines. (#20)
• Provide a simple backup system. (#22)
• Provide an upspin tree for managing GitHub issues. (#23)
• Support syncing from remote locations (Sydney / Mountain View).
• Support working on upspin trees with standard tools like Photoshop. (#24)
• Provide a simple setup process for new users.
• Support working in disconnected environments (airplanes).
• Provide a mechanism for key recovery.
• Use an upspin directory as $HOME (stretch)

For some reason this saved before I'd written anything. Here's what I was about to write:

In many of the tests, there are helpers with names like assertEquals or assertErrorEquals. These all need either to be deleted (which I prefer) or fixed to report the location of the caller. When a test fails, these helpers do not report the site of the failure, only the line number of the assert helper. I've seen test failures with methods as bad as,

foo_test.go:127: expected true, got false

with no indication wherever of where true came from or false should have come from.

There is also far too much dependence on golden errors. That's a different problem but also needs to change.

So we can see what the performance impact of a GCS backing is and other important metrics.

It now does not even check that the calling user has the right to see the system. It at least needs to restrict access to the owner.

But it should also implement Access files.

As of 75c2e9c the integration tests are failing.

Did they ever work with the new dir/server implementation? I seem to recall they did. @edpin?

--- FAIL: TestIntegration (71.22s)
    --- FAIL: TestIntegration/kind=gcp (71.20s)
        --- FAIL: TestIntegration/kind=gcp/packing=plain/curve=p256 (29.29s)
            --- FAIL: TestIntegration/kind=gcp/packing=plain/curve=p256/NoReadersAllowed (5.82s)
                integration_test.go:82: Expected error
            --- FAIL: TestIntegration/kind=gcp/packing=plain/curve=p256/AllowListAccess (1.94s)
                integration_test.go:109: Expected 1 entry, got 0
            --- FAIL: TestIntegration/kind=gcp/packing=plain/curve=p256/GlobWithLimitedAccess (1.13s)
                integration_test.go:191: got: [email protected]/dir2/file2.txt
                integration_test.go:193: reader globbing [email protected]/dir2/*.txt saw 1 dirs, want 0
        --- FAIL: TestIntegration/kind=gcp/packing=debug/curve=p256 (17.29s)
            --- FAIL: TestIntegration/kind=gcp/packing=debug/curve=p256/NoReadersAllowed (1.32s)
                integration_test.go:82: Expected error
            --- FAIL: TestIntegration/kind=gcp/packing=debug/curve=p256/AllowListAccess (0.90s)
                integration_test.go:109: Expected 1 entry, got 0
            --- FAIL: TestIntegration/kind=gcp/packing=debug/curve=p256/GlobWithLimitedAccess (1.21s)
                integration_test.go:191: got: [email protected]/dir2/file2.txt
                integration_test.go:193: reader globbing [email protected]/dir2/*.txt saw 1 dirs, want 0
        --- FAIL: TestIntegration/kind=gcp/packing=ee/curve=p256 (24.62s)
            --- FAIL: TestIntegration/kind=gcp/packing=ee/curve=p256/NoReadersAllowed (2.29s)
                integration_test.go:85: Expected error contains "permission denied", got "client.Get:\n\[email protected]/dir1/file1.txt: clientutil.ReadAll:\n\tuser [email protected]: pack/ee.Unpack: could not find wrapped key"
            --- FAIL: TestIntegration/kind=gcp/packing=ee/curve=p256/AllowListAccess (1.09s)
                integration_test.go:109: Expected 1 entry, got 0
            --- FAIL: TestIntegration/kind=gcp/packing=ee/curve=p256/GlobWithLimitedAccess (1.33s)
                integration_test.go:191: got: [email protected]/dir2/file2.txt
                integration_test.go:193: reader globbing [email protected]/dir2/*.txt saw 1 dirs, want 0
FAIL
exit status 1
FAIL    upspin.io/test  71.304s