scmp: add authenticated error messages using SPAO #4255
Conversation
There was a problem hiding this comment.
Great stuff!
I've not looked at the braccept tests in detail yet (:100: for adding these), but I saw that the actual authenticator tag is not checked there. That seems ok, but then we really need to have some unit tests for the computation of the MAC itself.
Reviewed 5 of 15 files at r1, all commit messages.
Reviewable status: 5 of 15 files reviewed, 12 unresolved discussions (waiting on @JordiSubira and @matzf)
pkg/slayers/extn.go line 28 at r1 (raw file):
var ( ErrOptionNotFound = serrors.New("Option not found") ZeroBlock [aes.BlockSize]byte
Unused.
pkg/slayers/pkt_auth.go line 272 at r1 (raw file):
// Compute inputLen = FixAuthDataInputLen + variable 3. + variable 4. (path) + // variable (Upper layer payload) // (https://scion.docs.anapaya.net/en/latest/protocols/authenticator-option.html#authenticated-data)
Nit, it's docs.scion.org now.
pkg/slayers/pkt_auth.go line 285 at r1 (raw file):
(opt.SPI().Type() == PacketAuthASHost && opt.SPI().Direction() == PacketAuthSenderSide) { inputLen += (int(scionL.DstAddrLen) + 1) * LineLen
Use addrBytes(), like below.
pkg/slayers/pkt_auth.go line 290 at r1 (raw file):
inputLen += scionL.Path.Len() inputLen += len(pld) input := make([]byte, inputLen)
Creating a buffer for the headers makes sense.
However, payload should not be copied into this buffer. It can be "written" directly to the CMAC. Ideally, we'd also avoid buffering the Path, but I'm not sure this would be easily possible.
At any rate, this input buffer be reusable to avoid allocating it for every call (either pass it as parameter or make this a member function of a struct that holds on to the buffer).
If the payload is not included in the buffer, there is a good upper bound for the required size of the buffer. Then we don't even need to compute the input length here.
pkg/slayers/pkt_auth.go line 296 at r1 (raw file):
} cmac.Write(input) copy(mac, cmac.Sum(nil))
Write to the mac buffer directly and return the resulting buffer from the function. This will append to the given output buffer and so will work even if the buffer is nil or does not have the correct length.
Change the function signature accordingly to return ([]byte, error).
Suggestion:
return cmac.Sum(mac[:0]), nilpkg/slayers/pkt_auth.go line 330 at r1 (raw file):
byte(s.SrcAddrType&0x3)<<2 | byte(s.SrcAddrLen&0x3) binary.BigEndian.PutUint16(buf[18:], 0) offset := 24
Shouldn't this be offset := 20 (== FixAuthDataInputLen)?
pkg/slayers/pkt_auth.go line 353 at r1 (raw file):
if err := s.Path.SerializeTo(buf[offset:]); err != nil { return err }
Mutable fields in the path must be zeroed out: CurrINF, CurrHF, SegID, router alert flags. Not sure what's the most efficient way to do this.
pkg/slayers/pkt_auth.go line 359 at r1 (raw file):
} func ComputeSPAOTimestamp(ts uint32) (uint32, error) {
The name of this function should somehow indicate that it creates a timestamp for the current time.
router/dataplane.go line 115 at r1 (raw file):
var ( drkeyDeriver = specific.Deriver{}
Must be a member of scionPacketProcessor (otherwise it would be used by multiple processors concurrently).
router/dataplane.go line 130 at r1 (raw file):
type drkeyProvider interface { GetSV() drkey.Key
This will need a timestamp parameter to find the SV for the correct epoch (e.g. when validating the traceroute requests).
router/dataplane.go line 1610 at r1 (raw file):
drkeyType := slayers.PacketAuthASHost if scmpH.TypeCode.Type() >= slayers.SCMPTypeEchoRequest && scmpH.TypeCode.Type() <= slayers.SCMPTypeTracerouteReply {
Traceroute Requests/Responses are authenticated with AS-Host keys, not Host-Host keys (because traceroute requests are not addressed to the router).
Also, Echo is not processed here. Instead, Echo requests are forwarded normally and will be received and processed by the end host stack running on the same machine.
router/dataplane.go line 1619 at r1 (raw file):
} timestamp, err := slayers.ComputeSPAOTimestamp(p.infoField.Timestamp)
p.infoField is the current info field, but the timestamp should be relative to info[0].
router/dataplane.go line 1649 at r1 (raw file):
if drkeyType == slayers.PacketAuthASHost { key, err := drkeyDeriver.DeriveASHost(dstA.String(), lvl1)
Maybe the DeriveASHost interface should be adapted to take the address in some non-stringy form, to avoid the formatting + parsing steps here.
This might not be super nice for now, we'll have some cleanup work to do with these address types anyway (see #4149).
There was a problem hiding this comment.
Reviewed 1 of 3 files at r2.
Reviewable status: 3 of 15 files reviewed, 16 unresolved discussions (waiting on @JordiSubira and @matzf)
pkg/slayers/pkt_auth.go line 86 at r2 (raw file):
// = PathMetaHdr + 3 IFs + 64 HFs // (see https://scion.docs.anapaya.net/en/latest/protocols/authenticator-option.html#authenticated-data) UpperBoundMACInput = 840
Use same name as used in other packages, MACBufferSize?
If it's already this big, perhaps just make it 12B (authenticator option meta) + 1020B (max SCION header length), then it will be sufficient for any possible path types.
Note: the documentation for the HdrLen field incorrectly says the max is 1024, but 255*4 == 1020. I'll fix that.
pkg/slayers/pkt_auth.go line 371 at r2 (raw file):
case *scion.Raw: // Zero out CurrInf && CurrHF p.Raw[0] = 0
This modifies the original path that will later also be used for sending, which would probably be bad.
If we're doing this raw buffer mangling anyway, it might make sense to write the path to the input data buffer first and then modify it in that buffer. That way, we don't even need the code path for Decoded.
pkg/slayers/pkt_auth.go line 409 at r2 (raw file):
return dec, nil default: return nil, serrors.New(fmt.Sprintf("unknown path type %T", orig))
The one hop path should also be handled.
The EPIC path type needs identical processing as SCION here, so we can perhaps support it without additional logic.
router/dataplane.go line 1607 at r2 (raw file):
error, ) { sv := p.drkeyProvider.GetSV(time.Now())
The timestamp used here here must match the timestamp included in the SPAO. Perhaps the slayers.ComputeSPAOCurrentTimestamp function interface needs to be adapted (again, sorry) so that the current time can be passed as argument.
There was a problem hiding this comment.
Yes, you're right I will add those UTs in the still missing
Reviewable status: 3 of 15 files reviewed, 14 unresolved discussions (waiting on @JordiSubira and @matzf)
pkg/slayers/pkt_auth.go line 285 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Use
addrBytes(), like below.
Done.
pkg/slayers/pkt_auth.go line 290 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Creating a buffer for the headers makes sense.
However, payload should not be copied into this buffer. It can be "written" directly to the CMAC. Ideally, we'd also avoid buffering the Path, but I'm not sure this would be easily possible.At any rate, this input buffer be reusable to avoid allocating it for every call (either pass it as parameter or make this a member function of a struct that holds on to the buffer).
If the payload is not included in the buffer, there is a good upper bound for the required size of the buffer. Then we don't even need to compute the input length here.
Done.
pkg/slayers/pkt_auth.go line 296 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Write to the
macbuffer directly and return the resulting buffer from the function. This will append to the given output buffer and so will work even if the buffer isnilor does not have the correct length.
Change the function signature accordingly to return([]byte, error).
Done.
pkg/slayers/pkt_auth.go line 330 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Shouldn't this be
offset := 20(== FixAuthDataInputLen)?
Done.
pkg/slayers/pkt_auth.go line 353 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Mutable fields in the path must be zeroed out: CurrINF, CurrHF, SegID, router alert flags. Not sure what's the most efficient way to do this.
Done.
pkg/slayers/pkt_auth.go line 359 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
The name of this function should somehow indicate that it creates a timestamp for the current time.
Done.
pkg/slayers/pkt_auth.go line 371 at r2 (raw file):
Previously, matzf (Matthias Frei) wrote…
This modifies the original path that will later also be used for sending, which would probably be bad.
If we're doing this raw buffer mangling anyway, it might make sense to write the path to the input data buffer first and then modify it in that buffer. That way, we don't even need the code path forDecoded.
Done.
pkg/slayers/pkt_auth.go line 409 at r2 (raw file):
Previously, matzf (Matthias Frei) wrote…
The one hop path should also be handled.
The EPIC path type needs identical processing as SCION here, so we can perhaps support it without additional logic.
EPIC still pending
router/dataplane.go line 115 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Must be a member of
scionPacketProcessor(otherwise it would be used by multiple processors concurrently).
Done.
router/dataplane.go line 130 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
This will need a timestamp parameter to find the SV for the correct epoch (e.g. when validating the traceroute requests).
Done.
router/dataplane.go line 1610 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Traceroute Requests/Responses are authenticated with AS-Host keys, not Host-Host keys (because traceroute requests are not addressed to the router).
Also, Echo is not processed here. Instead, Echo requests are forwarded normally and will be received and processed by the end host stack running on the same machine.
Done.
router/dataplane.go line 1619 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
p.infoFieldis the current info field, but the timestamp should be relative toinfo[0].
Done.
router/dataplane.go line 1607 at r2 (raw file):
Previously, matzf (Matthias Frei) wrote…
The timestamp used here here must match the timestamp included in the SPAO. Perhaps the
slayers.ComputeSPAOCurrentTimestampfunction interface needs to be adapted (again, sorry) so that the current time can be passed as argument.
Done.
pkg/slayers/extn.go line 28 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Unused.
Done.
There was a problem hiding this comment.
Reviewed 1 of 3 files at r2, 1 of 4 files at r3, 2 of 5 files at r4, all commit messages.
Reviewable status: 5 of 18 files reviewed, 9 unresolved discussions (waiting on @JordiSubira and @matzf)
pkg/slayers/pkt_auth.go line 359 at r1 (raw file):
Previously, JordiSubira wrote…
Done.
With the added now field, the name no longer really fits. Maybe call it relative timestamp?
Btw. this needs documentation, especially for the ts parameter.
pkg/slayers/pkt_auth.go line 409 at r2 (raw file):
Previously, JordiSubira wrote…
EPIC still pending
Handle can be handled in the SCION case statement (case *scion.Raw, *scion.Decoded, *epic.Path), just add something like if epic { offset += epic.MetadataLen } at the beginning
pkg/slayers/pkt_auth.go line 85 at r4 (raw file):
// 3. SCION Address Header // 4. Path // (see https://scion.docs.anapaya.net/en/latest/protocols/authenticator-option.html#authenticated-data)
Nit, url again, docs.scion.org
pkg/slayers/pkt_auth.go line 340 at r4 (raw file):
addrLen := addrBytes(s.DstAddrLen) copy(buf[offset:offset+addrLen], s.RawDstAddr) offset += addrLen
Could we just do copy directly? Is there any risk here that the length of RawDstAddr might not match DstAddrLen (and if so, what would the appropriate action be)?
Suggestion:
offset += copy(buf[offset::], s.RawDstAddr)pkg/slayers/pkt_auth.go line 366 at r4 (raw file):
} func zeroOutMutablePath(orig path.Path, buff []byte) error {
Nit, buf, not buff
pkg/slayers/pkt_auth.go line 374 at r4 (raw file):
// TODO(JordiSubira): process EPIC case *scion.Raw:
Combine cases to avoid the duplicate logic:
case *scion.Raw, *scion.Decoded:
...
pkg/slayers/pkt_auth_test.go line 301 at r4 (raw file):
return } goldenFile := "testdata/" + xtest.SanitizedName(t)
These regression tests give us assurance that we're not unintentionally breaking this later, and this can be useful, especially for covering a wider range of cases. Not sure this is enough here; we'd like to have some assertion that our implementation computes the MAC as required by the specification, at least for some base cases. I think we should prepare a few cases where we manually, byte for byte type out the expected CMAC input and then compare the resulting MACs.
On the other hand, we can simplify things a tiny bit in the setup for the tests, because the Keys that we use don't need to be actual DRKeys. We can just pick a key, just like now you've picked the master secret, and so we don't need to worry about the key derivation steps. For the regression tests, that also decouples the test output from changes in the drkey derivation.
router/dataplane.go line 1588 at r4 (raw file):
return nil, serrors.Wrap(cannotRoute, err, "details", "computing CMAC") } copy(optAuth.Authenticator(), mac)
That's a nop, source and destination slice are identical.
router/dataplane.go line 1637 at r4 (raw file):
// with timestamp is always unique sn := uint32(0) optAuth, err := slayers.NewPacketAuthOption(slayers.PacketAuthOptionParams{
Ideally, this PacketAuthOption object would be recycled (using Reset()). This should also allow to get rid of the temporary allocation for macBuf in this function here.
There was a problem hiding this comment.
Reviewable status: 5 of 18 files reviewed, 5 unresolved discussions (waiting on @matzf)
pkg/slayers/pkt_auth.go line 359 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
With the added
nowfield, the name no longer really fits. Maybe call it relative timestamp?Btw. this needs documentation, especially for the
tsparameter.
Done.
pkg/slayers/pkt_auth.go line 409 at r2 (raw file):
Previously, matzf (Matthias Frei) wrote…
Handle can be handled in the SCION case statement (
case *scion.Raw, *scion.Decoded, *epic.Path), just add something likeif epic { offset += epic.MetadataLen }at the beginning
Done.
pkg/slayers/pkt_auth.go line 340 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Could we just do copy directly? Is there any risk here that the length of
RawDstAddrmight not matchDstAddrLen(and if so, what would the appropriate action be)?
Yes, I think we can assume it. I made a minimal data-flow path inspection and it seems this is always set in the DecodeAddrHdr(·)
pkg/slayers/pkt_auth.go line 374 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Combine cases to avoid the duplicate logic:
case *scion.Raw, *scion.Decoded: ...
Done. I packed the duplicated in a function.
pkg/slayers/pkt_auth_test.go line 301 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
These regression tests give us assurance that we're not unintentionally breaking this later, and this can be useful, especially for covering a wider range of cases. Not sure this is enough here; we'd like to have some assertion that our implementation computes the MAC as required by the specification, at least for some base cases. I think we should prepare a few cases where we manually, byte for byte type out the expected CMAC input and then compare the resulting MACs.
On the other hand, we can simplify things a tiny bit in the setup for the tests, because the Keys that we use don't need to be actual DRKeys. We can just pick a key, just like now you've picked the master secret, and so we don't need to worry about the key derivation steps. For the regression tests, that also decouples the test output from changes in the drkey derivation.
What do you think if we tackle the "manually"-checked tests in a separate PR?
Done, removing the key derivation.
There was a problem hiding this comment.
Reviewed 5 of 7 files at r5, all commit messages.
Reviewable status: 7 of 19 files reviewed, 1 unresolved discussion (waiting on @JordiSubira and @matzf)
pkg/slayers/pkt_auth_test.go line 301 at r4 (raw file):
Previously, JordiSubira wrote…
What do you think if we tackle the "manually"-checked tests in a separate PR?
Done, removing the key derivation.
Hmm not sure, we don't have any checks to be confident that we're computing the MAC correctly. Or did you verify this manually in some form?
If this is about the size of the PR, I'd rather suggest to split the implementation of the MAC computation in pkg/slayers here into a first PR and move the the implementation in the router and the corresponding braccept tests into a second PR.
There was a problem hiding this comment.
Reviewable status: 7 of 19 files reviewed, 1 unresolved discussion (waiting on @JordiSubira and @matzf)
pkg/slayers/pkt_auth_test.go line 301 at r4 (raw file):
Previously, matzf (Matthias Frei) wrote…
Hmm not sure, we don't have any checks to be confident that we're computing the MAC correctly. Or did you verify this manually in some form?
If this is about the size of the PR, I'd rather suggest to split the implementation of the MAC computation inpkg/slayershere into a first PR and move the the implementation in the router and the corresponding braccept tests into a second PR.
Yes, it was about the size, but you're probably right, it does make sense to have the tests in this PR. Also, I guess since we have already reviewed the rest of the files, I'll keep the PR as it is and will add the tests.
There was a problem hiding this comment.
Reviewable status: 2 of 19 files reviewed, 1 unresolved discussion (waiting on @matzf)
pkg/slayers/pkt_auth_test.go line 301 at r4 (raw file):
Previously, JordiSubira wrote…
Yes, it was about the size, but you're probably right, it does make sense to have the tests in this PR. Also, I guess since we have already reviewed the rest of the files, I'll keep the PR as it is and will add the tests.
Done. I removed the regression tests which doesn't seem to add anything to these manual UTs.
There was a problem hiding this comment.
Reviewed 2 of 7 files at r6, all commit messages.
Reviewable status: 4 of 19 files reviewed, 12 unresolved discussions (waiting on @JordiSubira)
pkg/slayers/pkt_auth.go line 340 at r6 (raw file):
(opt.SPI().Type() == PacketAuthASHost && opt.SPI().Direction() == PacketAuthReceiverSide) { offset += copy(buf[offset:], s.RawDstAddr)
Bug. With AS-Host key and receiver-side key derivation, we must include the source address in the MAC input (the destination address is already part of the key derivation).
Same below for sender-side, where the destination address should be included in the MAC input.
pkg/slayers/pkt_auth.go line 402 at r6 (raw file):
for i := 0; i < base.NumINF; i++ { // Zero out IF.SegID buf[offset+2] = 0
Bug, SegID is two bytes that need to be zeroed.
pkg/slayers/pkt_auth_test.go line 42 at r6 (raw file):
algo = slayers.PacketAuthSHA1_AES_CBC ts = binary.LittleEndian.Uint32([]byte{1, 2, 3, 0}) sn = binary.LittleEndian.Uint32([]byte{4, 5, 6, 0})
Why specify explicitly as little endian? That just seems needlessly confusing, because in the packet it will be big endian. Easiest option is just write it as hex ts = 0x030201, sn = 0x060504 .
pkg/slayers/pkt_auth_test.go line 47 at r6 (raw file):
dir = slayers.PacketAuthSenderSide epoch = slayers.PacketAuthLater drkeyType = slayers.PacketAuthASHost
No need to specify these globally. The drkey options should be explicitly defined in the individual test cases.
pkg/slayers/pkt_auth_test.go line 50 at r6 (raw file):
decodedPath = &scion.Decoded{ Base: scion.Base{ PathMeta: scion.MetaHdr{
Set CurrINF/CurrHF to non-zero to test the zero-ing.
pkg/slayers/pkt_auth_test.go line 59 at r6 (raw file):
{ ConsDir: false, SegID: 1,
Use some SegID value with two non-zero bytes (to check the zeroing of both bytes)
Suggestion:
SegID: 0xf00f,pkg/slayers/pkt_auth_test.go line 83 at r6 (raw file):
ExpTime: 63, ConsIngress: 3, ConsEgress: 2,
Set router alert in some hop fields to test the zeroing.
pkg/slayers/pkt_auth_test.go line 230 at r6 (raw file):
testCases := map[string]struct { input []byte
Does not need to be in this struct, can be allocated in the test function below.
pkg/slayers/pkt_auth_test.go line 268 at r6 (raw file):
// 1. 0xff, 0xca, 0x0, 0xc, 0x0, 0x0, 0x3, 0xe8, 0x0, 0x6, 0x5, 0x4, // 2
It would be nice to split this into lines of "line length" (4 bytes) to match the description in the specifications for easier visual parsing.
Also annotate the individual fields so that the thing can be "verified" by careful inspection. See proposal below:
There are two bugs (see comments, also added comments to the responsible code).
Suggestion:
// 1. Authenticator Option Metadata
0xff, 0xca, 0x0, 0xc, // HdrLen | Upper Layer | Upper-Layer Packet Length
0x0, 0x0, 0x3, 0xe8, // Algorithm | Timestamp
0x0, 0x6, 0x5, 0x4, // RSV | Sequence Number
// 2. SCION Common Header
0x3, 0xf0, 0x12, 0x34, // Version | QoS | FlowID
0x1, 0x0, 0x0, 0x0, // PathType |DT |DL |ST |SL | RSV
// 3. SCION Address Header (only DST addr, because sender-side AS-Host key)
0xc0, 0x0, 0x0, 0x2, // <--- BUG!! this should be DST addr, but its SRC addr.
// Zeroed-out path
0x0, 0x0, 0x10, 0x41, // Path Meta Header (CurrINF, CurrHF = 0)
0x0, 0x0, 0x0, 0x1, // Info[0] (SegID = 0)
0x0, 0x3, 0x2, 0x1,
0x0, 0x0, 0x0, 0x2, // Info[1] (SegID = 0)
0x0, 0x3, 0x2, 0x1,
0x1, 0x0, 0x0, 0x3, // Info[2] (SegID = 0) <-- BUG!! SegID not zero
0x0, 0x3, 0x0, 0x1,
0x0, 0x3f, 0x0, 0x1, // Hop[0] (ConsIngress/Egress Alert = 0)
0x0, 0x0, 0x1, 0x2,
0x3, 0x4, 0x5, 0x6,
0x0, 0x3f, 0x0, 0x3, // Hop[1] (ConsIngress/Egress Alert = 0)
0x0, 0x2, 0x0, 0x2,
0x3, 0x4, 0x5, 0x6,
0x0, 0x3f, 0x0, 0x0, // Hop[2] (ConsIngress/Egress Router Alert = 0)
0x0, 0x2, 0x1, 0x2,
0x3, 0x4, 0x5, 0x6,pkg/slayers/pkt_auth_test.go line 456 at r6 (raw file):
macBuf := make([]byte, 16) spi, err := slayers.MakePacketAuthSPIDRKey(uint16(drkey.SCMP), drkeyType, dir, epoch)
We don't care about testing these functions here in this test, and so we can make things a bit more straight forward by explicitly specifying the relative timestamp and the SPI. This can be specified as the PacketAuthOptionParams struct for the individual test cases and so we don't need this getSPAO function.
Code quote:
spi, err := slayers.MakePacketAuthSPIDRKey(uint16(drkey.SCMP), drkeyType, dir, epoch)
assert.NoError(t, err)
timestamp, err := slayers.ComputeSPAORelativeTimestamp(ts, now)
assert.NoError(t, err)router/dataplane.go line 1612 at r6 (raw file):
func (p *scionPacketProcessor) getAuthKey(validTime time.Time) (drkey.Key, error) { sv := p.drkeyProvider.GetSV(validTime) dstA, err := p.scionLayer.SrcAddr()
The value is the same, but it would be more logical to use the destination IA/address of the scionL prepared in the calling function. Can either pass in the layer object or the IA/addr.
router/dataplane.go line 1628 at r6 (raw file):
} func (p *scionPacketProcessor) setSPAO(now time.Time) error {
Nit. Just from the name it's very hard to tell what this function does, i.e. that it resets only the metadata but not the MAC. Maybe something like resetSPAOMetadata would be appropriate?
There was a problem hiding this comment.
Reviewable status: 4 of 19 files reviewed, 11 unresolved discussions (waiting on @JordiSubira)
pkg/slayers/pkt_auth.go line 340 at r6 (raw file):
Previously, matzf (Matthias Frei) wrote…
Bug. With AS-Host key and receiver-side key derivation, we must include the source address in the MAC input (the destination address is already part of the key derivation).
Same below for sender-side, where the destination address should be included in the MAC input.
Nope, sorry, I mixed things up.
pkg/slayers/pkt_auth_test.go line 268 at r6 (raw file):
Previously, matzf (Matthias Frei) wrote…
It would be nice to split this into lines of "line length" (4 bytes) to match the description in the specifications for easier visual parsing.
Also annotate the individual fields so that the thing can be "verified" by careful inspection. See proposal below:There are two bugs (see comments, also added comments to the responsible code).
As commented above, the src/dst address in the MAC input is actually correct and my comment was wrong.
There was a problem hiding this comment.
Reviewable status: 3 of 19 files reviewed, 10 unresolved discussions (waiting on @matzf)
pkg/slayers/pkt_auth.go line 402 at r6 (raw file):
Previously, matzf (Matthias Frei) wrote…
Bug, SegID is two bytes that need to be zeroed.
Done.
pkg/slayers/pkt_auth_test.go line 42 at r6 (raw file):
Previously, matzf (Matthias Frei) wrote…
Why specify explicitly as little endian? That just seems needlessly confusing, because in the packet it will be big endian. Easiest option is just write it as hex
ts = 0x030201, sn = 0x060504.
Done.
pkg/slayers/pkt_auth_test.go line 47 at r6 (raw file):
Previously, matzf (Matthias Frei) wrote…
No need to specify these globally. The drkey options should be explicitly defined in the individual test cases.
Done.
pkg/slayers/pkt_auth_test.go line 50 at r6 (raw file):
Previously, matzf (Matthias Frei) wrote…
Set CurrINF/CurrHF to non-zero to test the zero-ing.
Done.
pkg/slayers/pkt_auth_test.go line 59 at r6 (raw file):
Previously, matzf (Matthias Frei) wrote…
Use some SegID value with two non-zero bytes (to check the zeroing of both bytes)
Done.
pkg/slayers/pkt_auth_test.go line 83 at r6 (raw file):
Previously, matzf (Matthias Frei) wrote…
Set router alert in some hop fields to test the zeroing.
Done.
pkg/slayers/pkt_auth_test.go line 230 at r6 (raw file):
Previously, matzf (Matthias Frei) wrote…
Does not need to be in this struct, can be allocated in the test function below.
Done.
pkg/slayers/pkt_auth_test.go line 268 at r6 (raw file):
Previously, matzf (Matthias Frei) wrote…
As commented above, the src/dst address in the MAC input is actually correct and my comment was wrong.
Done.
pkg/slayers/pkt_auth_test.go line 456 at r6 (raw file):
Previously, matzf (Matthias Frei) wrote…
We don't care about testing these functions here in this test, and so we can make things a bit more straight forward by explicitly specifying the relative timestamp and the SPI. This can be specified as the
PacketAuthOptionParamsstruct for the individual test cases and so we don't need thisgetSPAOfunction.
Done.
router/dataplane.go line 1612 at r6 (raw file):
Previously, matzf (Matthias Frei) wrote…
The value is the same, but it would be more logical to use the destination IA/address of the
scionLprepared in the calling function. Can either pass in the layer object or the IA/addr.
Done.
There was a problem hiding this comment.
Reviewed 4 of 6 files at r8, all commit messages.
Reviewable status: 6 of 20 files reviewed, 12 unresolved discussions (waiting on @JordiSubira)
pkg/slayers/pkt_auth.go line 306 at r8 (raw file):
} func SerializeAutenticatedData(
Keep this private (plus typo, authenticated). Add var SerializeAuthenticatedData = serializeAuthenticatedData to export_test.go.
pkg/slayers/pkt_auth.go line 314 at r8 (raw file):
buf[0] = byte(CmnHdrLen + s.AddrHdrLen() + s.Path.Len()) buf[1] = byte(L4SCMP)
This must be the protocol type of the actual packet. As this requires ignoring the extension headers, we don't want to extract this from the SCION data and so this needs to be a parameter here (pldType L4ProtocolType).
pkg/slayers/pkt_auth.go line 366 at r8 (raw file):
} func TimeFromRelativeTimeStamp(spaoTS uint32, ts uint32) time.Time {
Nit. TimeFromSPAORelativeTimestamp.
Timestamp (not TimeStamp) for consistency.
Arguments order reversed from ComputeSPAORelativeTimestamp (info[0].Timestamp is first)?
pkg/slayers/pkt_auth_test.go line 50 at r6 (raw file):
Previously, JordiSubira wrote…
Done.
0xab is a weird value for CurrINF because it cannot be represented in the two bit wire value.
Same for 0xcd which is not a valid CurrHF value.
pkg/slayers/pkt_auth_test.go line 234 at r8 (raw file):
pld []byte rawMACInput []byte assertFormatErr assert.ErrorAssertionFunc
Suggestion:
assertErr assert.ErrorAssertionFuncrouter/dataplane.go line 1537 at r8 (raw file):
if cause != nil || (scmpH.TypeCode.Type() == slayers.SCMPTypeTracerouteReply && p.hasValidAuth()) {
Ideally we'd like to reuse the key from the auth valididty check for the reply. Maybe mention this in a comment if you want to defer this optimization for later.
Nit. Just assign the needsAuth bool directly, no if.
router/dataplane.go line 1654 at r8 (raw file):
return err } return nil
Just return p.optAuth.Reset(...)
router/dataplane.go line 1665 at r8 (raw file):
); err != nil { return false }
This doesn't seem to check whether the E2E extension header is present. It could happen that we parse a UDP or SCMP payload or the HBH as the E2E.
During SCION packet processing we use the EndToEndExtnSkipper layer. This doesn't parse the options at all, but does keep a reference to the data slice if the extension header is present. We can create the EndToEndExtn from that (from p.e2eLayer.Contents).
The existing p.e2eLayer is not reset between different packets, you'll have to add this (p.e2eLayer = slayers.EndToEndExtnSkipper{}).
Note: ideally we'd have some sort of a scanning implementation of FindOption method for EndToEndExtnSkipper to avoid parsing all the tlvOptions, but we can do that as an optimisation later on.
router/dataplane.go line 1685 at r8 (raw file):
} // the sender should have use the receiver side key, i.e., K_{localIA-remoteIA:remoteHost} // where remoteIA == p.scionLayer.SrcIA and remoteHost == dstAddr
Confusing renaming. Why not just keep calling it srcAddr?
router/dataplane.go line 1695 at r8 (raw file):
authOption, &p.scionLayer, p.e2eLayer.Payload, // scmpH + scmpP
I'd use p.lastLayer.LayerPayload() here (same thing, but IMO clarifies the intent).
The comment // scmpH + scmpP honestly just confused me.
router/dataplane.go line 1704 at r8 (raw file):
// compare incoming authField with computed authentication tag return bytes.Equal(authOption.Authenticator(), p.validAuthBuf)
subtle.ConstantTimeCompare() != 0.
tools/braccept/cases/scmp_dest_unreachable.go line 176 at r8 (raw file):
packAuthOpt.EndToEndOption, }, }
Move the construction of e2e into a helper function in scmp.go, e.g. func normalizedSCMPPacketAuth() *slayers.EndToEndExtn.
This is absolutely identical for all cases that check SCMPs errors.
There was a problem hiding this comment.
Reviewable status: 3 of 21 files reviewed, 11 unresolved discussions (waiting on @matzf)
pkg/slayers/pkt_auth.go line 306 at r8 (raw file):
Previously, matzf (Matthias Frei) wrote…
Keep this private (plus typo, authenticated). Add
var SerializeAuthenticatedData = serializeAuthenticatedDatatoexport_test.go.
Done.
pkg/slayers/pkt_auth.go line 314 at r8 (raw file):
Previously, matzf (Matthias Frei) wrote…
This must be the protocol type of the actual packet. As this requires ignoring the extension headers, we don't want to extract this from the
SCIONdata and so this needs to be a parameter here (pldType L4ProtocolType).
Done.
pkg/slayers/pkt_auth_test.go line 50 at r6 (raw file):
Previously, matzf (Matthias Frei) wrote…
0xabis a weird value forCurrINFbecause it cannot be represented in the two bit wire value.
Same for0xcdwhich is not a validCurrHFvalue.
Done.
pkg/slayers/pkt_auth_test.go line 234 at r8 (raw file):
pld []byte rawMACInput []byte assertFormatErr assert.ErrorAssertionFunc
Done.
router/dataplane.go line 1537 at r8 (raw file):
Previously, matzf (Matthias Frei) wrote…
Ideally we'd like to reuse the key from the auth valididty check for the reply. Maybe mention this in a comment if you want to defer this optimization for later.
Nit. Just assign the
needsAuthbool directly, no if.
Done.
router/dataplane.go line 1654 at r8 (raw file):
Previously, matzf (Matthias Frei) wrote…
Just
return p.optAuth.Reset(...)
Done.
router/dataplane.go line 1665 at r8 (raw file):
Previously, matzf (Matthias Frei) wrote…
This doesn't seem to check whether the E2E extension header is present. It could happen that we parse a UDP or SCMP payload or the HBH as the E2E.
During SCION packet processing we use the
EndToEndExtnSkipperlayer. This doesn't parse the options at all, but does keep a reference to the data slice if the extension header is present. We can create theEndToEndExtnfrom that (fromp.e2eLayer.Contents).
The existingp.e2eLayeris not reset between different packets, you'll have to add this (p.e2eLayer = slayers.EndToEndExtnSkipper{}).Note: ideally we'd have some sort of a scanning implementation of
FindOptionmethod forEndToEndExtnSkipperto avoid parsing all the tlvOptions, but we can do that as an optimisation later on.
Done.
router/dataplane.go line 1685 at r8 (raw file):
Previously, matzf (Matthias Frei) wrote…
Confusing renaming. Why not just keep calling it
srcAddr?
Done.
router/dataplane.go line 1695 at r8 (raw file):
Previously, matzf (Matthias Frei) wrote…
I'd use
p.lastLayer.LayerPayload()here (same thing, but IMO clarifies the intent).The comment
// scmpH + scmpPhonestly just confused me.
Done.
router/dataplane.go line 1704 at r8 (raw file):
Previously, matzf (Matthias Frei) wrote…
subtle.ConstantTimeCompare() != 0.
Done.
tools/braccept/cases/scmp_dest_unreachable.go line 176 at r8 (raw file):
Previously, matzf (Matthias Frei) wrote…
Move the construction of
e2einto a helper function inscmp.go, e.g.func normalizedSCMPPacketAuth() *slayers.EndToEndExtn.
This is absolutely identical for all cases that check SCMPs errors.
Done.
There was a problem hiding this comment.
Reviewed 1 of 15 files at r1, 3 of 7 files at r6, 1 of 6 files at r8, 13 of 13 files at r9, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @JordiSubira)
pkg/slayers/pkt_auth.go line 306 at r8 (raw file):
Previously, JordiSubira wrote…
Done.
Still a typo. And btw. why not a shorter alias with var ... instead of re-declaring the function in export_test.go?
router/dataplane.go line 1706 at r9 (raw file):
// Reset p.e2eLayer p.e2eLayer = slayers.EndToEndExtnSkipper{}
This should be in scionPacketProcessor.reset; and perhaps reset the hbh layer too for consistency.
Btw. resetting this is not strictly required with the lastLayer.CanDecode(...) check that you do now at the beginning of the function. I had had a different check in mind (something like len(p.e2eLayer.Content) > 0) where resetting would have been crucial.
tools/braccept/main.go line 84 at r9 (raw file):
cases.SCMPTracerouteIngressWithSPAO(artifactsDir, hfMAC), // cases.ParentToChild(artifactsDir, hfMAC),
Don't forget to re-enable these before merging.
tools/braccept/cases/scmp_expired_hop.go line 137 at r9 (raw file):
ReadFrom: "veth_131_host", Input: input.Bytes(), Want: nil,
Huh, that's a catch. I hadn't really noticed before that we can't send SCMPs for (very) expired segments because the timestamp cannot be represented.
Some time ago, @fstreun had proposed to represent the timestamps relative to the drkey epochs instead of relative to the info field (see #4062 (comment) and following discussion). This would be a fix for this...
tools/braccept/cases/scmp_traceroute.go line 362 at r9 (raw file):
} // Skip Ethernet + IPv4 + UDP
Nit. Wrongly copy pasted comment?
There was a problem hiding this comment.
Reviewable status: 15 of 21 files reviewed, 3 unresolved discussions (waiting on @matzf)
pkg/slayers/pkt_auth.go line 306 at r8 (raw file):
Previously, matzf (Matthias Frei) wrote…
Still a typo. And btw. why not a shorter alias with
var ...instead of re-declaring the function in export_test.go?
Done.
router/dataplane.go line 1706 at r9 (raw file):
Previously, matzf (Matthias Frei) wrote…
This should be in
scionPacketProcessor.reset; and perhaps reset the hbh layer too for consistency.Btw. resetting this is not strictly required with the
lastLayer.CanDecode(...)check that you do now at the beginning of the function. I had had a different check in mind (something likelen(p.e2eLayer.Content) > 0) where resetting would have been crucial.
Done.
tools/braccept/main.go line 84 at r9 (raw file):
Previously, matzf (Matthias Frei) wrote…
Don't forget to re-enable these before merging.
Done.
tools/braccept/cases/scmp_expired_hop.go line 137 at r9 (raw file):
Previously, matzf (Matthias Frei) wrote…
Huh, that's a catch. I hadn't really noticed before that we can't send SCMPs for (very) expired segments because the timestamp cannot be represented.
Some time ago, @fstreun had proposed to represent the timestamps relative to the drkey epochs instead of relative to the info field (see #4062 (comment) and following discussion). This would be a fix for this...
Sure, we can tackle this as was discussed in this thread. I will do it in a future commit if that's fine.
|
@JordiSubira, you'll need to rebase on master. With #4297, the buildkite runs should work again. |
There was a problem hiding this comment.
Reviewed 6 of 6 files at r10, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @JordiSubira)
There was a problem hiding this comment.
Reviewed 1 of 15 files at r1, 1 of 7 files at r6, 1 of 6 files at r8, 8 of 13 files at r9, 6 of 6 files at r10.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @JordiSubira)
pkg/slayers/pkt_auth.go line 79 at r10 (raw file):
// FixAuthDataInputLen is the unvariable fields length for the // authenticated data FixAuthDataInputLen = MinPacketAuthDataLen + 8
Make both of these constants private, nobody needs these now.
pkg/slayers/pkt_auth.go line 271 at r10 (raw file):
} func ComputeAuthCMAC(
This should have docs. Suggestion:
// ComputeAuthCMAC computes the authenticator tag for the AES-CMAC algorithm.
// The key should correspond to the SPI defined in opt.SPI.
// The SCION layer, payload type and payload define the input to the MAC, as defined in https://docs.scion.org/en/latest/protocols/authenticator-option.html#authenticated-data.
//
// The input buffer is used as a temporary buffer for the MAC computation. It must be at least MACBufferSize long.
// The resulting MAC is written to macBuffer (appending, if necessary), and returned as a slice of length 16.pkg/slayers/pkt_auth.go line 281 at r10 (raw file):
) ([]byte, error) { // TODO(matzf): avoid allocations, somehow?
Remove comment
pkg/slayers/pkt_auth.go line 381 at r10 (raw file):
} switch p := orig.(type) { // TODO(JordiSubira): process EPIC
Remove comment
pkg/slayers/pkt_auth.go line 405 at r10 (raw file):
} func zeroOutWithBase(base scion.Base, offset int, buf []byte) {
Nit, tiny bit more elegant would be to only have buf parameter and pass in buf[epic.MetadataLen:] for Epic.
JordiSubira
force-pushed
the
scmp_impl
branch
from
8ea0867
to
1b20de8
Compare
There was a problem hiding this comment.
Reviewable status: 15 of 21 files reviewed, 4 unresolved discussions (waiting on @matzf)
pkg/slayers/pkt_auth.go line 79 at r10 (raw file):
Previously, matzf (Matthias Frei) wrote…
Make both of these constants private, nobody needs these now.
Done.
pkg/slayers/pkt_auth.go line 271 at r10 (raw file):
Previously, matzf (Matthias Frei) wrote…
This should have docs. Suggestion:
// ComputeAuthCMAC computes the authenticator tag for the AES-CMAC algorithm. // The key should correspond to the SPI defined in opt.SPI. // The SCION layer, payload type and payload define the input to the MAC, as defined in https://docs.scion.org/en/latest/protocols/authenticator-option.html#authenticated-data. // // The input buffer is used as a temporary buffer for the MAC computation. It must be at least MACBufferSize long. // The resulting MAC is written to macBuffer (appending, if necessary), and returned as a slice of length 16.
Done.
pkg/slayers/pkt_auth.go line 281 at r10 (raw file):
Previously, matzf (Matthias Frei) wrote…
Remove comment
Done.
pkg/slayers/pkt_auth.go line 381 at r10 (raw file):
Previously, matzf (Matthias Frei) wrote…
Remove comment
Done.
There was a problem hiding this comment.
Reviewed 6 of 6 files at r11, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @JordiSubira)
pkg/slayers/pkt_auth.go line 337 at r11 (raw file):
binary.BigEndian.PutUint32(buf[12:], firstHdrLine) buf[16] = byte(s.PathType) buf[17] = byte(s.DstAddrType&0x3)<<6 | byte(s.DstAddrType.Length()&0x3)<<4 |
Length returns the length in bytes which is not the value that should be encoded here. Accidentally passes the tests because the value is truncated back to 0 which is correct for IPv4; would not work for IPv6.
The type/length components don't need to be encoded separately (see #4160):
Suggestion:
buf[17] = byte(s.DstAddrType&0x7)<<4 | byte(s.SrcAddrType&0x7)pkg/slayers/pkt_auth_test.go line 251 at r11 (raw file):
DstIA: dstIA, SrcAddrType: slayers.T4Ip, RawSrcAddr: net.IPv4(192, 0, 0, 2).To4(),
See comment above related to encoding address type; could you change one of the test cases to use IPv6 and one to use a SVC address, to get broader coverage?
There was a problem hiding this comment.
Reviewable status: 19 of 21 files reviewed, 2 unresolved discussions (waiting on @matzf)
pkg/slayers/pkt_auth.go line 337 at r11 (raw file):
Previously, matzf (Matthias Frei) wrote…
Length returns the length in bytes which is not the value that should be encoded here. Accidentally passes the tests because the value is truncated back to 0 which is correct for IPv4; would not work for IPv6.
The type/length components don't need to be encoded separately (see #4160):
Done, it was an oversight from my side, sorry for the confussion.
pkg/slayers/pkt_auth_test.go line 251 at r11 (raw file):
Previously, matzf (Matthias Frei) wrote…
See comment above related to encoding address type; could you change one of the test cases to use IPv6 and one to use a SVC address, to get broader coverage?
Done.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r12, all commit messages.
Reviewable status:
There was a problem hiding this comment.
Reviewed 1 of 3 files at r2, 4 of 7 files at r6, 1 of 6 files at r8, 6 of 13 files at r9, 3 of 6 files at r10, 4 of 6 files at r11, 2 of 2 files at r12, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @JordiSubira)
pkg/slayers/pkt_auth.go line 280 at r12 (raw file):
// The resulting MAC is written to macBuffer (appending, if necessary), // and returned as a slice of length 16. func ComputeAuthCMAC(
I think this function is doing too much. (Which is also indicated by the number of arguments)
I would split this into three parts:
- Constructing cipher
- Computing the MAC input
- Calculating the MAC
Out of these 3 things, I think only the second one belongs conceptually into the slayers package.
Slayers and subpackages are designed as a parsing/encoding layers. They should not do any complex logic.
(Currently we have the infamous path.Path.Reverse method which violates this rule)
I suggest we create a new SPAO related package pkg/spao where we can move cipher construction (if even necessary)
As for the calculating the MAC: I don't think these needs to be abstracted in a function and can be done by the caller directly.
pkg/slayers/pkt_auth.go line 367 at r12 (raw file):
// now = ts+spaoTS⋅𝑞, (where q := 6 ms and ts = info[0].Timestamp, i.e., // the timestamp field in the first InfoField). func ComputeSPAORelativeTimestamp(ts uint32, now time.Time) (uint32, error) {
I think this would also be a candidate for the spao package alluded to above.
That would also make for nicer names on the caller side spao.RelativeTimestamp()
pkg/slayers/pkt_auth.go line 378 at r12 (raw file):
// then = ts + spaoTS⋅𝑞, (where q := 6 ms and ts = info[0].Timestamp, i.e., // the timestamp field in the first InfoField). func TimeFromRelativeTimestamp(ts uint32, spaoTS uint32) time.Time {
This function name is too generic. There is nothing that ties it to spao.
If we would move it to the spao package, this would read very well as spao.Time()
router/dataplane.go line 130 at r12 (raw file):
// zeroBuffer will be used to reset the Authenticator option in the // scionPacketProcessor.OptAuth zeroBuffer = make([]byte, 16)
I would allocate a zeroBuffer per packet processor.
I'm not exactly sure what the performance implications are of sharing this buffer, if the go routines are scheduled on different cores.
router/dataplane.go line 133 at r12 (raw file):
) type drkeyProvider interface {
This fits on one line.
Suggestion:
type drkeyProvider interface {
GetAuthKey(validTime time.Time, dstIA addr.IA, dstAddr net.Addr) (drkey.Key, error)
}
There was a problem hiding this comment.
Reviewable status: 13 of 23 files reviewed, 5 unresolved discussions (waiting on @matzf and @oncilla)
pkg/slayers/pkt_auth.go line 280 at r12 (raw file):
Previously, oncilla (Dominik Roos) wrote…
I think this function is doing too much. (Which is also indicated by the number of arguments)
I would split this into three parts:
- Constructing cipher
- Computing the MAC input
- Calculating the MAC
Out of these 3 things, I think only the second one belongs conceptually into the slayers package.
Slayers and subpackages are designed as a parsing/encoding layers. They should not do any complex logic.
(Currently we have the infamouspath.Path.Reversemethod which violates this rule)I suggest we create a new SPAO related package
pkg/spaowhere we can move cipher construction (if even necessary)As for the calculating the MAC: I don't think these needs to be abstracted in a function and can be done by the caller directly.
Done.
pkg/slayers/pkt_auth.go line 367 at r12 (raw file):
Previously, oncilla (Dominik Roos) wrote…
I think this would also be a candidate for the
spaopackage alluded to above.
That would also make for nicer names on the caller sidespao.RelativeTimestamp()
Done.
pkg/slayers/pkt_auth.go line 378 at r12 (raw file):
Previously, oncilla (Dominik Roos) wrote…
This function name is too generic. There is nothing that ties it to spao.
If we would move it to the spao package, this would read very well as
spao.Time()
Done.
router/dataplane.go line 130 at r12 (raw file):
Previously, oncilla (Dominik Roos) wrote…
I would allocate a zeroBuffer per packet processor.
I'm not exactly sure what the performance implications are of sharing this buffer, if the go routines are scheduled on different cores.
I can change it, but how is this different from sharing the errors above from different go routines? This buffer is expected to be read-only after initialization.
router/dataplane.go line 133 at r12 (raw file):
Previously, oncilla (Dominik Roos) wrote…
This fits on one line.
Done.
There was a problem hiding this comment.
Reviewed 10 of 10 files at r13, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @JordiSubira)
pkg/spao/timestamp.go line 15 at r13 (raw file):
// limitations under the License. package spao
Add a package level doc comment.
I think at least we should mention what spao stands for and a link to the related documentation pages
(see: https://go.dev/blog/godoc)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @JordiSubira)
router/dataplane.go line 130 at r12 (raw file):
Previously, JordiSubira wrote…
I can change it, but how is this different from sharing the errors above from different go routines? This buffer is expected to be read-only after initialization.
Conceptually, no difference. I'm also not sure if the have a performance impact.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @JordiSubira)
pkg/slayers/pkt_auth.go line 280 at r12 (raw file):
Previously, JordiSubira wrote…
Done.
While I don't like the long list of parameters either, I don't really agree; why is it better to move this logic out to the caller? It's the same logic for every caller.
Especially if we now move things to a spao package, I think it makes more sense to have the full ComputeAuthCMAC function to implement the entire logic for the AES-CMAC algorithm.
IMO, there should be a separate function for every SPAO algorithm type. If you look at SHA1-AES-CBC, the only other algorithm type currently specified, to me it seems quite clear that computing the hash and MAC is exactly something that the library should be taking care of, not the caller.
pkg/spao/timestamp.go line 15 at r13 (raw file):
// limitations under the License. package spao
If we have this spao package, then I think the entire content of pkt_auth.go file should be moved here (and renamed) too. In particular, the types PacketAuthOption (just spao.Option?), PacketAuthAlg, PacketAuthSPI, etc. and all the constants.
There was a problem hiding this comment.
Reviewable status: 16 of 27 files reviewed, 2 unresolved discussions (waiting on @matzf and @oncilla)
pkg/slayers/pkt_auth.go line 280 at r12 (raw file):
Previously, matzf (Matthias Frei) wrote…
While I don't like the long list of parameters either, I don't really agree; why is it better to move this logic out to the caller? It's the same logic for every caller.
Especially if we now move things to a
spaopackage, I think it makes more sense to have the fullComputeAuthCMACfunction to implement the entire logic for theAES-CMACalgorithm.
IMO, there should be a separate function for every SPAO algorithm type. If you look atSHA1-AES-CBC, the only other algorithm type currently specified, to me it seems quite clear that computing the hash and MAC is exactly something that the library should be taking care of, not the caller.
Okay, not too convinced about the organization of this. At the moment, I have decided to move the logic for the ComputeAuthCMAC to the spao package and leave the rest of the parsing in the slayers package. After revisiting it, I think it makes sense to encapsulate everything in the ComputeAuthCMAC as the input serialization will vary depending on the MAC algorithm used.
pkg/spao/timestamp.go line 15 at r13 (raw file):
Previously, oncilla (Dominik Roos) wrote…
Add a package level doc comment.
I think at least we should mention what spao stands for and a link to the related documentation pages(see: https://go.dev/blog/godoc)
done.
pkg/spao/timestamp.go line 15 at r13 (raw file):
Previously, matzf (Matthias Frei) wrote…
If we have this spao package, then I think the entire content of
pkt_auth.gofile should be moved here (and renamed) too. In particular, the typesPacketAuthOption(justspao.Option?),PacketAuthAlg,PacketAuthSPI, etc. and all the constants.
See comment on the pkt_auth.go file.
There was a problem hiding this comment.
Reviewed 3 of 10 files at r13, 11 of 11 files at r14, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @JordiSubira and @oncilla)
pkg/slayers/pkt_auth.go line 280 at r12 (raw file):
Previously, JordiSubira wrote…
Okay, not too convinced about the organization of this. At the moment, I have decided to move the logic for the
ComputeAuthCMACto thespaopackage and leave the rest of the parsing in theslayerspackage. After revisiting it, I think it makes sense to encapsulate everything in theComputeAuthCMACas the input serialization will vary depending on the MAC algorithm used.
Ok, the ComputeAuthCMAC interface looks good to me.
I'd be interested to know what the reason for your choice to not move the parsing logic and type definitions to spao too.
pkg/slayers/pkt_auth.go line 78 at r14 (raw file):
// We round this up to 12B (authenticator option meta) + 1020B (max SCION header length) // To adapt to any possible path types. MACBufferSize = 1032
These three constants should go to the spao package (they are all concerned with the ComputeAuthCMAC functionality). Keep fixAuthDataInputLen private.
pkg/spao/doc.go line 21 at r14 (raw file):
// It provides support for the MAC and the timestamp computations // used when utilizing the SPAO header.
Remove this newline here (otherwise it's not recognized as the package doc by godoc).
There was a problem hiding this comment.
Reviewable status: 21 of 27 files reviewed, 3 unresolved discussions (waiting on @matzf and @oncilla)
pkg/slayers/pkt_auth.go line 280 at r12 (raw file):
Previously, matzf (Matthias Frei) wrote…
Ok, the ComputeAuthCMAC interface looks good to me.
I'd be interested to know what the reason for your choice to not move the parsing logic and type definitions to
spaotoo.
I decided to keep the SPAO layout and the parsing methods for the header in the same package as the rest of the parsing for the other headers, as @oncilla mentioned in the past. I am open to move everything to the spao package, but I think it falls under the current criteria for slayers. Anyway, if someone has a stronger argument to move it outside the spao package, I will do it.
pkg/slayers/pkt_auth.go line 78 at r14 (raw file):
Previously, matzf (Matthias Frei) wrote…
These three constants should go to the
spaopackage (they are all concerned with the ComputeAuthCMAC functionality). KeepfixAuthDataInputLenprivate.
Done. I kept the AuthOptionMetadataLen as I keep at the moment the layout and parsing logic for spao here. As mentioned below, I do not have a strong opinion about it and open to change it.
pkg/spao/doc.go line 21 at r14 (raw file):
Previously, matzf (Matthias Frei) wrote…
Remove this newline here (otherwise it's not recognized as the package doc by godoc).
Done.
There was a problem hiding this comment.
Reviewed 6 of 6 files at r15, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla)
pkg/slayers/pkt_auth.go line 280 at r12 (raw file):
Previously, JordiSubira wrote…
I decided to keep the SPAO layout and the parsing methods for the header in the same package as the rest of the parsing for the other headers, as @oncilla mentioned in the past. I am open to move everything to the spao package, but I think it falls under the current criteria for slayers. Anyway, if someone has a stronger argument to move it outside the spao package, I will do it.
Ok, thanks! Sure, I agree that it also makes sense to be in slayers, and I don't object to leaving this here for now.
For the record, I think the arguments in favor (+) / against (-) moving the packet-auth option definitions and parsing functionality to the spao package are:
+keep the SPAO related things together, as they will be used together-keep all packet parsing related things together inslayers; so a user of the library can quickly find all the things related to parsing a packet -- not very strong point IMO, as the TLV options only need to be parsed for processing (otherwise can be left opaque).+allows shorter names / lessPacketAuth...-clutter inslayerspackage-churn; no strong reason to move this now, can just as well move it later
pkg/slayers/pkt_auth.go line 78 at r14 (raw file):
Previously, JordiSubira wrote…
Done. I kept the
AuthOptionMetadataLenas I keep at the moment the layout and parsing logic for spao here. As mentioned below, I do not have a strong opinion about it and open to change it.
Nit. Should be named either PacketAuthOptionMetadataLen or PacketAuthMetadataLen for consistency.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r16, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla)
There was a problem hiding this comment.
Reviewed 5 of 11 files at r14, 4 of 6 files at r15, 2 of 2 files at r16, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @JordiSubira)
pkg/spao/mac.go line 112 at r16 (raw file):
pld []byte, ) (int, error) {
Given this will be on the "fast path" in the router, I think it would make sense to do bound check elimination.
https://go101.org/article/bounds-check-elimination.html
I suspect the compiler is not smart enough and will do many bound checks here. (Would need to verify this with the appropriate compiler flags or VS code extension though)
(non-blocking, needs to be benchmarked to justify)
There was a problem hiding this comment.
Reviewable status:
adapt code linting + comments failing int-test r2 remove unnecessary failing SCMP expired-hop tests add UT mac computation gazelle r4 gazelle add manual UTs delete regression UT r6 add validation for traceroute request lint r8 r9 revision rebased missing revision review remove ComputAuthCMAC revision missing file Revert "remove ComputAuthCMAC" This reverts commit 664b4b9. refactor slayers/spao packages lint add doc review pass review
There was a problem hiding this comment.
Reviewed 1 of 1 files at r17, all commit messages.
Reviewable status:
JordiSubira
force-pushed
the
scmp_impl
branch
from
bcc628b
to
35e484b
Compare
There was a problem hiding this comment.
Reviewed 1 of 13 files at r9, 2 of 10 files at r13, 3 of 11 files at r14, 2 of 6 files at r15, 1 of 2 files at r16, 1 of 1 files at r17, 13 of 13 files at r18, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @JordiSubira and @oncilla)
pkg/spao/doc.go line 15 at r18 (raw file):
// limitations under the License. // Package spao implements the logic needed to provide support to the
"support for"
pkg/spao/mac.go line 159 at r18 (raw file):
err := orig.SerializeTo(buf) if err != nil { return serrors.WrapStr("serializing path for reseting fields", err)
resetting
router/dataplane.go line 72 at r18 (raw file):
// e2eAuthHdrLen is the length in bytes of added information when a SCMP packet // needs to be authenticated: 16B (e2e.option.Len()) + 16 B(CMAC_tag.Len()).
16B (CMAC_tag.Len())
router/dataplane.go line 1637 at r18 (raw file):
// Error messages must be authenticated. // Traceroute are OPTIONALLY authenticated ONLY IF the Request
request
router/dataplane.go line 1690 at r18 (raw file):
key, err := p.drkeyProvider.GetAuthKey(now, scionL.DstIA, srcA) if err != nil { return nil, err
Wrap error with cannotRoute and "details"?
router/dataplane.go line 1693 at r18 (raw file):
} if err := p.resetSPAOMetadata(now); err != nil { return nil, err
Wrap error with cannotRoute and "details"?
router/dataplane.go line 1728 at r18 (raw file):
// For creating SCMP responses we use sender side. dir := slayers.PacketAuthSenderSide // TODO(JordiSubira): At the moment, We assume the later epoch at the moment.
Remove "At the moment, "
tools/braccept/cases/scmp_expired_hop.go line 144 at r18 (raw file):
// SCMPExpiredHopMessageBack tests a packet with an expired hop field. The relative timestamp // can be encoded in the SPAO header and send back to the src.
"sent back"? (multiple times in this file)
There was a problem hiding this comment.
Reviewed 4 of 4 files at r19, all commit messages.
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
router/dataplane.go line 72 at r18 (raw file):
Previously, marcfrei (Marc Frei) wrote…
16B (CMAC_tag.Len())
Done.
router/dataplane.go line 1637 at r18 (raw file):
Previously, marcfrei (Marc Frei) wrote…
request
Done.
There was a problem hiding this comment.
Reviewable status:
This PR adds the SPAO header to SCMP error messages:
braccepttests.Still missing:
This change is