On 07/08/2011 06:20 PM, Jeff Darcy wrote:
Hello.
I have some comments..
The new at-rest encryption code requires three parts:
* A high-level protocol between client and server
* A low-level cryptographic protocol on the client
* A lease/queue management system on the server
In the interests of having an implementation of the first part that at
least compiles and runs (and in fact actually passes some pretty
devious tests), I've posted an implementation of the "crypt" and
"oplock" translators to the "cbc" branch of the git repository.
This
is not intended to replace other work in these areas, but it does
identify and solve many non-obvious problems with the higher-level
protocol so that ongoing work can focus on the other areas. For
posterity, here's a full description of the design and some notes about
the state of the code.
= HIGH LEVEL PROTOCOL DESIGN =
The goal of the high-level protocol is to prevent data corruption that
might occur when multiple clients attempt to do concurrent
read-modify-write sequences as required by block encryption. Clients
use a three-part sequence for unaligned writes:
* Acquire a lease and fetch any unaligned portion at the beginning of
the data area they'll be writing.
* Write the data, including merged and re-encrypted portions including
the unaligned portions.
* Give up the lease and deal with end-of-file issues.
More specifically, the lease operations involve a "magic" extended
attribute of the form trusted.crypt.OFFSET-LENGTH-HEAD-TAIL, where the
fields in the last part have the following meanings:
OFFSET is the beginning of the write in the next step, shifted
left if unaligned
LENGTH is the length of the write in the next step, shifted
right if unaligned
HEAD is the number of bytes for the server to return at the
beginning of the range defined by OFFSET and LENGTH
TAIL is the number of bytes for the server to return at the
end of the range defined by OFFSET and LENGTH
It seems we came to an agreement that passing such "normal
parameters" is meaningless: since we implement file leasing,
it is impossible to construct a unique lease id by them..
I think client should pass a 128-bit generation counter. And
server should append (or prepends) a unique client id.
The lease-acquire operation is an fgetxattr.
Why?
I find its signature rather improper for this:
typedef int32_t (*fop_fgetxattr_t) (call_frame_t *frame,
xlator_t *this,
fd_t *fd,
const char *name);
How are we going to pass the magic name and encoded value
via ->fgetxattr()?
I suggest to consider ->fsetxattr() for this purpose..
The returned value is in (up to)
two parts, "head" and "tail" representing the
chunks requested by
HEAD and/or
TAIL if either is non-zero. If this request conflicts with a
sequence already
in progress, it should be queued for continuation when the sequence
completes.
The write operation must exactly match the HEAD and OFFSET values in
order to
be considered a continuation of the same read-modify-sequence;
otherwise, it
will be considered a separate write and queued behind the sequence
already in
progress.
The lease-relinquish operation
Do we really need the above?
oplock->writev_cbk(), and oplock->truncate_cbk can drop
the lease. We must know the atom size on the server side
though (to recognize RMWs). But IMHO this is not a
problem (we can store this in xattrs (**) for example..)
is an fsetxattr, using the same attribute name
as before, with the value being the real EOF (which might be prior to
what
was written). At the completion of this operation, the server should
issue
the next previously-blocked conflicting request.
Fully aligned writes participate in the leasing/queuing process, but
use a
slightly optimized protocol. The client only issues the write
itself; the
lease acquisition and relinquishment are implicit, and are handled
within the
server.
If write is fully aligned (i.e. not RMW), then it
was issued without any lease and oplock->writev_cbk()
drops nothing. Yes, it can be considered as a kind
of participation..
Reads must also be aware of this protocol, similarly to aligned
writes. When
a read is received which conflicts with an in-progress write or write
sequence,
it is queued and continued when the write completes.
I don't see any reasons why reads must be aware of this
protocol. What problems are resolved by making them aware
of this? We do know what are conflicting writes, but what
are conflicting read and write?
These are leases, not locks. If an entire fgetxattr/writev/fsetxattr
sequence
is not completed within the lease period, the client may retry at its
own
discretion. The server is responsible for handling lease expiration,
including
the continuation of already-queued requests as appropriate.
The server is required to deal with end-of-file issues related to the
cipher-block granularity of writes. At the conclusion of an
unaligned-write
sequence, the server must store the "extra" bytes (beyond
EOF as sent
in the
closing message) in an xattr and truncate the file to the correct
length. If
such an "end fragment" xattr already exists and the file is
being
extended,
the xattr contents must be written to their correct location within
the file.
The same promotion from xattr to file contents must also be done if
an
*aligned* write extends the file.
= LOW LEVEL (CRYPTOGRAPHIC) PROTOCOL DESIGN =
This is the design that was developed with the assistance of Robert
Relyea
from Red Hat's security team, who deserves our thanks. The basic
mechanism
employed is block-level encryption (currently AES) with per-file keys
and
per-cipherblock IVs which are generated using the per-file keys. This
combination is believed to be effective against known attacks that
have plagued
previous approaches, including:
* keystream recovery using known plaintext (no keystream)
* watermarking between files (per-file keys)
* watermarking within a file (no relationship between block IVs)
* bit-flipping (any change affects whole cipher-block and no other)
When a file is created, a new random key is generated and encrypted
using the
global key. The result is then stored on an xattr, which may be
fetched and
decrypted on subsequent opens. Each time a cipher-block is written,
it is
encrypted as follows:
IV = hash(file_key||block_num)
ciphertext = encrypt(plaintext,file_key,IV)
The reversal should be obvious to one who has the file key, which in turn
requires having the global key.
= CODE STATUS =
The code in the "cbc" branch implements the entire high-level
protocol
(both
sides) as decribed above. It has passed the following tests:
* A "combo" test which tests various cases of different alignments and
writes which do or do not span more than one cipher-block. First it
tests writes from one byte up to the cipher-block size at the
beginning of a file, then cipher-block-size writes at offsets from
one to cipher-block size, and finally decreasing sizes down to one
byte at the end of the file.
* A "frag" test which exercises a problematic data-overwrite case not
exercised by the above.
* A program which writes a file using one prime block size, then copies
that file using a different prime block size, all writes using an
in-program implementation of the same cyrptographic protocol as we're
using ourselves. This exercises more alignment, block-spanning,
overwrite and hole-filling cases.
* Building (not running) iozone. Linkers have notoriously weird
access patterns, and was the last test to complete successfully.
Many subtle problems, especially to do with holes and end fragments, were
uncovered by these tests. I very strongly recommend that anyone
developing a
separate implementation of the protocols described here run at least
these
tests (which I'll make available somehow) or their equivalent.
Any
code that
fails these tests, for which solutions are already known and
published, will be
NAKed with extreme prejudice.
While this code does implement the full read and write paths of the
high-level
protocol, it is incomplete or immature in other ways. The following
items are
not yet implemented.
* Read-modify-write sequences require promotion of a user's O_RDONLY
to O_RDWR, but that in turn requires checking of subsequent writes
(and other modifying operations such as truncate) to preserve the
user-visible O_RDONLY behavior. This access control is not yet
implemented.
* The (f)truncate paths need to deal with end fragments, and are not
yet implemented to do so.
* Only the direct encryption part of the low-level crypto protocol
have been implemented. Generation of per-file keys and calculation
of per-block IVs remain TBD.
* The server-side lease and queue management is not yet implemented.
* No lease-timeout retries are done on the client. This might not
need to be done, because conflict of any kind should be rare and the
only likely cause for a lease timeout would be loss of connection.
That's already a disaster in the non-replication case, and we
shouldn't be trying to make it better. In the replication case,
we need to consider situations such as the write part of a
read-modify-write going to a server that knows nothing about our
previous fgetxattr (which went to a now-failed server) and is
processed as a "naked" fully-aligned write. In the very rare case
where two clients hit this simultaneously, their writes might not
be properly serialized and data loss or truncation could result.
Lastly, here's a list of general code-hygiene issues that need to be
addressed
(some of these apply to other code as well).
* End-fragment xattrs should be removed when they're no longer valid,
This makes things queasy. Every such removal can cause rebalance of
local-fs storage. Could you please remind why to not have files with
rounded-up sizes on server? After all we can keep their actual size
in xattrs and to not bother with its removal...
Thanks,
Edward.
e.g. when the file is extended or truncated.
* The code is undoubtedly full of memory leaks. The "life cycle" of
various objects - e.g. fd, inode, dict, iobref - needs to be managed
more carefully with references added and removed (including error
paths).
* It never hurts to make sure that error codes are being checked and
reported properly throughout the code, and there are at least a few
(marked) cases where errors in secondary operations such as
end-fragment moves are being ignored.
* The code almost certainly won't compile without warnings on 32-bit
systems, and might not compile (or work) on those systems at all.