Hello Jan,

Nice to feel your engagement, thanks for the comments.

On Wed, Apr 20, 2016 at 11:50:00AM -0400, Jan Harkes wrote:
> > For the record, there exists our yet-unpublished Aetey/Chalmers branch,
> > differing quite a bit from 6.9.5, with
> > 
> > - the s.k. modular clog included, with proper integration with Kerberos
> >   and support for multiple authentication authorities (this appeared
> >   at CMU long ago but never made it to upstream)
> > - server IP addresses can be changed without disrupting client
> >   operation and without client-side administration, removing the afs-derived
> >   "static server IP" contraint
> > - clents can handle servers running on non-standard ports (as a result
> >   e.g. more than one server can be put behind the same NAT)
> > - a bunch of small fixes and tweaks
> 
> I've seen the modular clog a long time ago and I remember it was not in
> a state to be easily merged, added a bunch of complexity and additional
> configuration for what I thought were not typical cases. The few

The additional configuration is optional if you do not need the new
features.

There remains the added complexity. That's why the code is modular, most
of it is in separate (and optional unless you need the functionality)
executables.

Finally, I guess the notion of typical Coda deployments has changed
since when you saw the code.

> comments I remember were,
> 
>     - the clog token file format was/is not portable across platforms
>       and needed to be cleaned up so that different endianess systems
>       could also read the tokens that were used, not everything is x86.

If this was an issue it presumably has been solved long ago too.

Peeking at the current code...
This does not look broken, must be from the time before the code
reached Aetey:
----
 ...
static void export(ClearToken *cToken)
{
    cToken->AuthHandle     = htonl(cToken->AuthHandle);
    cToken->ViceId         = htonl(cToken->ViceId);
    cToken->BeginTimestamp = htonl(cToken->BeginTimestamp);
    cToken->EndTimestamp   = htonl(cToken->EndTimestamp);
}

static void import(ClearToken *cToken)
{
    cToken->AuthHandle     = ntohl(cToken->AuthHandle);
    cToken->ViceId         = ntohl(cToken->ViceId);
    cToken->BeginTimestamp = ntohl(cToken->BeginTimestamp);
    cToken->EndTimestamp   = ntohl(cToken->EndTimestamp);
}
 ...
void WriteTokenToBytestream(FILE *f, ClearToken *cToken,
                      EncryptedSecretToken sToken)
{
    char *buf;
    int len;

    len = sizeof(ClearToken) + sizeof(EncryptedSecretToken);
    buf = malloc(len);
    export(cToken);
    memcpy(buf, (char *)cToken, sizeof(ClearToken));
    memcpy(buf + sizeof(ClearToken), sToken, sizeof(EncryptedSecretToken));
    import(cToken);

    fputs("*** Coda Token ***", f);
    coda_base64_encode(f, buf, len);
    free(buf);
}
 ...
----

FWIIW we are running the same code on ARM.

The only problem we hit there was not with clog but with memory alignment
in cfs (which we then fixed).

>     - there was some sort of configuration daemon serving up
>       configuration data for the authentication from a tcp port which
>       introduces a whole slew of unevaluated security concerns. Since it
>       is http how is the configuration secured from MitM attacks, can

There is no http involved (it would be a large overhead without any reason).

This data does not have to be secured against MitM, the worst which can
happen is disruption of communication / DOS which MitM always can achieve.

>       someone replace authentication with an auth-null forcing the
>       clients to suddenly send out cleartext passwords, etc. And if it
>       is secured (HMAC?), how do client know how to properly vet the
>       signature, etc.

There is no support for sending cleartext passwords so no danger of that
sort either. It is an explicit design choice, authentication methods
not providing reliable server authentication to the client are forbidden.
Actually, any method which could send a password to the server should
be banned as well and none of the built ones does, of course.

> As far as the other changes, I haven't seen any of them but I can still
> throw out some unsollicited comments.
> 
> Allowing server IP addresses to change without client-side intervention
> has to introduce some new level of indirection above the IP layer and
> the most obvious one is to refer to servers using their DNS names. This
> works nicely with the 'new' RPC2 call I added almost 10 years ago.
> 
>     https://github.com/cmusatyalab/coda/commit/86d97f6db3ac13d6d83f47636e23891f9380f537

Even something which looks obvious can be deceiving.

For reference here is an excerpt from my letter to this list, 2010,
outlining one of the problems with it:
----
 ...

Even today, a server's invariant "identity" in a realm is not it's ipv4
address but it's server id.

 ...

Today we translate them to ip addresses (at wrong place but it is another
part of the story) without any possibility of the addresses' expiration
even though their exact values are not guaranteed to last forever nor
are relevant for realm consistency.

With ViceGetVolumeLocation() we make a similar mistake (!), we do
not allow for the returned string to be invalidated "properly". There
should have been some kind of a validity promise (say a TTL) included.
A possibility to change host names in a realm setup without confusing
the clients is a Good Thing (TM), why forbid this by design?
 ...
----

So this would replace the "constant ip numbers" constraint with
"constant host names", why keep such a constraint at all? From system
administration perspective this easily becomes a PITA in the long run
when networking changes, dns domain names change and naming policies
change as well.

The only string which really has to remain stable is the realm name,
as the handle for reaching the service and the data.

Actually, the whole concept of a "hostname" is broken, DNS purpose
is to map _service_ names to endpoints (ip+port) of the corresponding
daemons.
Hardware units or OS instances (aka hosts) are irrelevant for this
(even if they looked "natural" long ago when IP networking was in
its infancy and uucp was state-of-the-art).

That's why we resolve the services, which are the numbered server
instances in a realm, to endpoints via SRV records, the exact tool for
the purpose.

This fixed the ip-dependency issue, still keeping compatibility and
without complexity. (The commit introducing the change makes the code
grow 105 lines, comparable to the much more limited in scope commit for
ViceGetVolumeLocation() which added 85 lines).

> The response to that RPC call even includes the port the server is
> running at. The only part that has not been implemented is the client
> side and it only got stalled because the available async DNS libraries
> were either not LWP compatible, or had a non-GPL compatible license. 
> Synchronous DNS blocks the complete client process causing various kinds
> of connectivity/reliability issues, so I am interested to see how you
> solved that part of the problem.

We did not see any practical problems or extra stalls caused by
synchronous DNS resolution. Definitely not an issue in our workloads.
Of course nothing precludes changing to asynchronous resolution if
needed but the effort and possible dependencies are hardly justified.

If not otherwise, the presence of callbacks is much more of a concern.
In a file system where clients go disconnected as a matter of normal
operation, callbacks do not give much benefit, at the same time callback
breaking _does_ cause stalls.

> > The best would be to merge the changes upstream. They are vital for
> > usability.
> 
> Unless you have managed to fix some horrible reintegration/resolution
> conflict introducting bug, I respectfully disagree with the 'vital' part
> of that statement. But merging upstream is for many things, especially
> bug fixes, always a good idea.

It would be quite sad if the qualities which are vital for us (and
credibly for someone else too, even if not for CMU) will be neglected.

> Well, github has the concept of pull requests. The best method seems to
> be to have small, independent, single purpose changes on 'feature
> branches' that can be easily read, reviewed and merged.

Ok.

> Right now I'm mostly concerned with cleaning things up and addressing
> some bugs and possible security issues I believe I have identified.

Great, thanks for looking into this.

> Anything that removes lines of code, and/or reduces overall complexity
> will be much easier to merge because it reduces the amount of cleanup
> necessary.

Looking forward to it. As a low-hanging fruit, would you object to
removing the hack which supports running "numbered" server instances on
the same computer, with different assigned ips and dns names?
I guess the motivation for it disappeared over 10 years ago when you
implemented coalescing of free space in RVM and made the servers a lot
more scalable.

> ps. About the 'as yet-unpublished' part of your comment, I do hope you
> are adhering to the GPL license, as you distribute binaries of Coda with
> extensive changes you are expected to make the corresponding source
> available. In fact as a company, you would be expected to make source
> available even if there are no extensive changes.

You have no reason to worry. When we distribute the binaries, we distribute
the source side-by-side with them.

At the same time, publishing one's development history (like a git
repository) is not something which GPL mandates :)

Regards,
Rune