Skip to content

Commit

Permalink
dhcpv4: conform to RFC 2131 with respect to options.
Browse files Browse the repository at this point in the history 
Removes AddOption and GetOption.

RFC 2131 specifies that options may only appear once (Section 4.1). If
an option does appear more than once, its byte values must be
concatenated.

RFC 3396 further specifies that to send options longer than 255 bytes,
one option may be split into multiple option codes, which must be
concatenated back together by the receiver.

Both of these are concerned with the byte representation of options.
Fact is, based on both RFCs one can say that an option may only appear
once, but may be composed of multiple values.

Because an option may appear only once logically in any case, we remove
the AddOption and GetOption functions and leave only UpdateOption and
GetOneOption.

Also remove all additions & checks of the End option - the marshaling
and unmarshaling code is exclusively responsible for that now.
  • Loading branch information
@hugelgupf
hugelgupf committed Jan 13, 2019
1 parent a955d74 commit c7b40dc
Show file tree
Hide file tree
Showing 10 changed files with 113 additions and 222 deletions.
59 changes: 28 additions & 31 deletions dhcpv4/bsdp/bsdp.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ type ReplyConfig struct {
// ParseBootImageListFromAck parses the list of boot images presented in the
// ACK[LIST] packet and returns them as a list of BootImages.
func ParseBootImageListFromAck(ack dhcpv4.DHCPv4) ([]BootImage, error) {
var images []BootImage
opt := ack.GetOneOption(dhcpv4.OptionVendorSpecificInformation)
if opt == nil {
return nil, errors.New("ParseBootImageListFromAck: could not find vendor-specific option")
Expand All @@ -40,11 +39,11 @@ func ParseBootImageListFromAck(ack dhcpv4.DHCPv4) ([]BootImage, error) {
if err != nil {
return nil, err
}
bootImageOpts := vendorOpt.GetOption(OptionBootImageList)
for _, opt := range bootImageOpts {
images = append(images, opt.(*OptBootImageList).Images...)
bootImageOpts := vendorOpt.GetOneOption(OptionBootImageList)
if bootImageOpts == nil {
return nil, fmt.Errorf("boot image option not found")
}
return images, nil
return bootImageOpts.(*OptBootImageList).Images, nil
}

func needsReplyPort(replyPort uint16) bool {
Expand All @@ -59,12 +58,14 @@ func MessageTypeFromPacket(packet *dhcpv4.DHCPv4) *MessageType {
vendorOpts *OptVendorSpecificInformation
err error
)
for _, opt := range packet.GetOption(dhcpv4.OptionVendorSpecificInformation) {
if vendorOpts, err = ParseOptVendorSpecificInformation(opt.ToBytes()); err == nil {
if o := vendorOpts.GetOneOption(OptionMessageType); o != nil {
if optMessageType, ok := o.(*OptMessageType); ok {
return &optMessageType.Type
}
opt := packet.GetOneOption(dhcpv4.OptionVendorSpecificInformation)
if opt == nil {
return nil
}
if vendorOpts, err = ParseOptVendorSpecificInformation(opt.ToBytes()); err == nil {
if o := vendorOpts.GetOneOption(OptionMessageType); o != nil {
if optMessageType, ok := o.(*OptMessageType); ok {
return &optMessageType.Type
}
}
}
Expand Down Expand Up @@ -114,21 +115,21 @@ func NewInformList(hwaddr net.HardwareAddr, localIP net.IP, replyPort uint16) (*
if needsReplyPort(replyPort) {
vendorOpts = append(vendorOpts, &OptReplyPort{replyPort})
}
d.AddOption(&OptVendorSpecificInformation{vendorOpts})
d.UpdateOption(&OptVendorSpecificInformation{vendorOpts})

d.AddOption(&dhcpv4.OptParameterRequestList{
d.UpdateOption(&dhcpv4.OptParameterRequestList{
RequestedOpts: []dhcpv4.OptionCode{
dhcpv4.OptionVendorSpecificInformation,
dhcpv4.OptionClassIdentifier,
},
})
d.AddOption(&dhcpv4.OptMaximumDHCPMessageSize{Size: MaxDHCPMessageSize})
d.UpdateOption(&dhcpv4.OptMaximumDHCPMessageSize{Size: MaxDHCPMessageSize})

vendorClassID, err := MakeVendorClassIdentifier()
if err != nil {
return nil, err
}
d.AddOption(&dhcpv4.OptClassIdentifier{Identifier: vendorClassID})
d.UpdateOption(&dhcpv4.OptClassIdentifier{Identifier: vendorClassID})
return d, nil
}

Expand Down Expand Up @@ -179,8 +180,8 @@ func InformSelectForAck(ack dhcpv4.DHCPv4, replyPort uint16, selectedImage BootI
if err != nil {
return nil, err
}
d.AddOption(&dhcpv4.OptClassIdentifier{Identifier: vendorClassID})
d.AddOption(&dhcpv4.OptParameterRequestList{
d.UpdateOption(&dhcpv4.OptClassIdentifier{Identifier: vendorClassID})
d.UpdateOption(&dhcpv4.OptParameterRequestList{
RequestedOpts: []dhcpv4.OptionCode{
dhcpv4.OptionSubnetMask,
dhcpv4.OptionRouter,
Expand All @@ -189,8 +190,8 @@ func InformSelectForAck(ack dhcpv4.DHCPv4, replyPort uint16, selectedImage BootI
dhcpv4.OptionClassIdentifier,
},
})
d.AddOption(&dhcpv4.OptMessageType{MessageType: dhcpv4.MessageTypeInform})
d.AddOption(&OptVendorSpecificInformation{vendorOpts})
d.UpdateOption(&dhcpv4.OptMessageType{MessageType: dhcpv4.MessageTypeInform})
d.UpdateOption(&OptVendorSpecificInformation{vendorOpts})
return d, nil
}

Expand All @@ -212,9 +213,9 @@ func NewReplyForInformList(inform *dhcpv4.DHCPv4, config ReplyConfig) (*dhcpv4.D
reply.ServerIPAddr = config.ServerIP
reply.ServerHostName = config.ServerHostname

reply.AddOption(&dhcpv4.OptMessageType{MessageType: dhcpv4.MessageTypeAck})
reply.AddOption(&dhcpv4.OptServerIdentifier{ServerID: config.ServerIP})
reply.AddOption(&dhcpv4.OptClassIdentifier{Identifier: AppleVendorID})
reply.UpdateOption(&dhcpv4.OptMessageType{MessageType: dhcpv4.MessageTypeAck})
reply.UpdateOption(&dhcpv4.OptServerIdentifier{ServerID: config.ServerIP})
reply.UpdateOption(&dhcpv4.OptClassIdentifier{Identifier: AppleVendorID})

// BSDP opts.
vendorOpts := []dhcpv4.Option{
Expand All @@ -226,9 +227,7 @@ func NewReplyForInformList(inform *dhcpv4.DHCPv4, config ReplyConfig) (*dhcpv4.D
if config.SelectedImage != nil {
vendorOpts = append(vendorOpts, &OptSelectedBootImageID{ID: config.SelectedImage.ID})
}
reply.AddOption(&OptVendorSpecificInformation{Options: vendorOpts})

reply.AddOption(&dhcpv4.OptionGeneric{OptionCode: dhcpv4.OptionEnd})
reply.UpdateOption(&OptVendorSpecificInformation{Options: vendorOpts})
return reply, nil
}

Expand All @@ -252,18 +251,16 @@ func NewReplyForInformSelect(inform *dhcpv4.DHCPv4, config ReplyConfig) (*dhcpv4
reply.ServerHostName = config.ServerHostname
reply.BootFileName = config.BootFileName

reply.AddOption(&dhcpv4.OptMessageType{MessageType: dhcpv4.MessageTypeAck})
reply.AddOption(&dhcpv4.OptServerIdentifier{ServerID: config.ServerIP})
reply.AddOption(&dhcpv4.OptClassIdentifier{Identifier: AppleVendorID})
reply.UpdateOption(&dhcpv4.OptMessageType{MessageType: dhcpv4.MessageTypeAck})
reply.UpdateOption(&dhcpv4.OptServerIdentifier{ServerID: config.ServerIP})
reply.UpdateOption(&dhcpv4.OptClassIdentifier{Identifier: AppleVendorID})

// BSDP opts.
reply.AddOption(&OptVendorSpecificInformation{
reply.UpdateOption(&OptVendorSpecificInformation{
Options: []dhcpv4.Option{
&OptMessageType{Type: MessageTypeSelect},
&OptSelectedBootImageID{ID: config.SelectedImage.ID},
},
})

reply.AddOption(&dhcpv4.OptionGeneric{OptionCode: dhcpv4.OptionEnd})
return reply, nil
}
21 changes: 6 additions & 15 deletions dhcpv4/bsdp/bsdp_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ func TestParseBootImageListFromAck(t *testing.T) {
},
}
ack, _ := dhcpv4.New()
ack.AddOption(&OptVendorSpecificInformation{
ack.UpdateOption(&OptVendorSpecificInformation{
[]dhcpv4.Option{&OptBootImageList{expectedBootImages}},
})

Expand Down Expand Up @@ -69,7 +69,6 @@ func TestNewInformList_NoReplyPort(t *testing.T) {
require.True(t, m.Options.Has(dhcpv4.OptionVendorSpecificInformation))
require.True(t, m.Options.Has(dhcpv4.OptionParameterRequestList))
require.True(t, m.Options.Has(dhcpv4.OptionMaximumDHCPMessageSize))
require.True(t, m.Options.Has(dhcpv4.OptionEnd))

opt := m.GetOneOption(dhcpv4.OptionVendorSpecificInformation)
require.NotNil(t, opt, "vendor opts not present")
Expand Down Expand Up @@ -108,8 +107,7 @@ func newAck(hwAddr net.HardwareAddr, transactionID [4]byte) *dhcpv4.DHCPv4 {
ack.TransactionID = transactionID
ack.HWType = iana.HWTypeEthernet
ack.ClientHWAddr = hwAddr
ack.AddOption(&dhcpv4.OptMessageType{MessageType: dhcpv4.MessageTypeAck})
ack.AddOption(&dhcpv4.OptionGeneric{OptionCode: dhcpv4.OptionEnd})
ack.UpdateOption(&dhcpv4.OptMessageType{MessageType: dhcpv4.MessageTypeAck})
return ack
}

Expand All @@ -127,7 +125,7 @@ func TestInformSelectForAck_Broadcast(t *testing.T) {
}
ack := newAck(hwAddr, tid)
ack.SetBroadcast()
ack.AddOption(&dhcpv4.OptServerIdentifier{ServerID: serverID})
ack.UpdateOption(&dhcpv4.OptServerIdentifier{ServerID: serverID})

m, err := InformSelectForAck(*ack, 0, bootImage)
require.NoError(t, err)
Expand All @@ -143,7 +141,6 @@ func TestInformSelectForAck_Broadcast(t *testing.T) {
require.True(t, m.Options.Has(dhcpv4.OptionDHCPMessageType))
opt := m.GetOneOption(dhcpv4.OptionDHCPMessageType)
require.Equal(t, dhcpv4.MessageTypeInform, opt.(*dhcpv4.OptMessageType).MessageType)
require.True(t, m.Options.Has(dhcpv4.OptionEnd))

// Validate vendor opts.
require.True(t, m.Options.Has(dhcpv4.OptionVendorSpecificInformation))
Expand Down Expand Up @@ -186,7 +183,7 @@ func TestInformSelectForAck_BadReplyPort(t *testing.T) {
}
ack := newAck(hwAddr, tid)
ack.SetBroadcast()
ack.AddOption(&dhcpv4.OptServerIdentifier{ServerID: serverID})
ack.UpdateOption(&dhcpv4.OptServerIdentifier{ServerID: serverID})

_, err := InformSelectForAck(*ack, 11223, bootImage)
require.Error(t, err, "expect error for > 1024 replyPort")
Expand All @@ -206,7 +203,7 @@ func TestInformSelectForAck_ReplyPort(t *testing.T) {
}
ack := newAck(hwAddr, tid)
ack.SetBroadcast()
ack.AddOption(&dhcpv4.OptServerIdentifier{ServerID: serverID})
ack.UpdateOption(&dhcpv4.OptServerIdentifier{ServerID: serverID})

replyPort := uint16(999)
m, err := InformSelectForAck(*ack, replyPort, bootImage)
Expand Down Expand Up @@ -281,9 +278,6 @@ func TestNewReplyForInformList(t *testing.T) {
RequireHasOption(t, ack.Options, &dhcpv4.OptClassIdentifier{Identifier: AppleVendorID})
require.NotNil(t, ack.GetOneOption(dhcpv4.OptionVendorSpecificInformation))

// Ensure options terminated with End option.
require.Equal(t, &dhcpv4.OptionGeneric{OptionCode: dhcpv4.OptionEnd}, ack.Options[len(ack.Options)-1])

// Vendor-specific options.
vendorOpts := ack.GetOneOption(dhcpv4.OptionVendorSpecificInformation).(*OptVendorSpecificInformation)
RequireHasOption(t, vendorOpts.Options, &OptMessageType{Type: MessageTypeList})
Expand Down Expand Up @@ -363,9 +357,6 @@ func TestNewReplyForInformSelect(t *testing.T) {
RequireHasOption(t, ack.Options, &dhcpv4.OptClassIdentifier{Identifier: AppleVendorID})
require.NotNil(t, ack.GetOneOption(dhcpv4.OptionVendorSpecificInformation))

// Ensure options are terminated with End option.
require.Equal(t, &dhcpv4.OptionGeneric{OptionCode: dhcpv4.OptionEnd}, ack.Options[len(ack.Options)-1])

vendorOpts := ack.GetOneOption(dhcpv4.OptionVendorSpecificInformation).(*OptVendorSpecificInformation)
RequireHasOption(t, vendorOpts.Options, &OptMessageType{Type: MessageTypeSelect})
RequireHasOption(t, vendorOpts.Options, &OptSelectedBootImageID{ID: images[0].ID})
Expand Down Expand Up @@ -424,7 +415,7 @@ func TestMessageTypeForPacket(t *testing.T) {
t.Run(tt.tcName, func(t *testing.T) {
pkt, _ = dhcpv4.New()
for _, opt := range tt.opts {
pkt.AddOption(opt)
pkt.UpdateOption(opt)
}
gotMessageType = MessageTypeFromPacket(pkt)
require.Equal(t, tt.wantMessageType, gotMessageType)
Expand Down
5 changes: 0 additions & 5 deletions dhcpv4/bsdp/option_vendor_specific_information.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,6 @@ func (o *OptVendorSpecificInformation) String() string {
return s
}

// GetOption returns all suboptions that match the given OptionCode code.
func (o *OptVendorSpecificInformation) GetOption(code dhcpv4.OptionCode) []dhcpv4.Option {
return o.Options.Get(code)
}

// GetOneOption returns the first suboption that matches the OptionCode code.
func (o *OptVendorSpecificInformation) GetOneOption(code dhcpv4.OptionCode) dhcpv4.Option {
return o.Options.GetOne(code)
Expand Down
57 changes: 0 additions & 57 deletions dhcpv4/bsdp/option_vendor_specific_information_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,52 +166,6 @@ func TestOptVendorSpecificInformationString(t *testing.T) {
require.Equal(t, expectedString, o.String())
}

func TestOptVendorSpecificInformationGetOptions(t *testing.T) {
// No option
o := &OptVendorSpecificInformation{
[]dhcpv4.Option{
&OptMessageType{MessageTypeList},
Version1_1,
},
}
foundOpts := o.GetOption(OptionBootImageList)
require.Empty(t, foundOpts, "should not get any options")

// One option
o = &OptVendorSpecificInformation{
[]dhcpv4.Option{
&OptMessageType{MessageTypeList},
Version1_1,
},
}
foundOpts = o.GetOption(OptionMessageType)
require.Equal(t, 1, len(foundOpts), "should only get one option")
require.Equal(t, MessageTypeList, foundOpts[0].(*OptMessageType).Type)

// Multiple options
//
// TODO: Remove this test when RFC 3396 is properly implemented. This
// isn't a valid packet. RFC 2131, Section 4.1: "Options may appear
// only once." RFC 3396 clarifies this to say that options will be
// concatenated. I.e., in this case, the bytes would be:
//
// <versioncode> 4 1 1 0 1
//
// Which would obviously not be parsed by the Version parser, as it
// only accepts two bytes.
o = &OptVendorSpecificInformation{
[]dhcpv4.Option{
&OptMessageType{MessageTypeList},
Version1_1,
Version1_0,
},
}
foundOpts = o.GetOption(OptionVersion)
require.Equal(t, 2, len(foundOpts), "should get two options")
require.Equal(t, Version1_1, foundOpts[0].(Version))
require.Equal(t, Version1_0, foundOpts[1].(Version))
}

func TestOptVendorSpecificInformationGetOneOption(t *testing.T) {
// No option
o := &OptVendorSpecificInformation{
Expand All @@ -232,15 +186,4 @@ func TestOptVendorSpecificInformationGetOneOption(t *testing.T) {
}
foundOpt = o.GetOneOption(OptionMessageType)
require.Equal(t, MessageTypeList, foundOpt.(*OptMessageType).Type)

// Multiple options
o = &OptVendorSpecificInformation{
[]dhcpv4.Option{
&OptMessageType{MessageTypeList},
Version1_1,
Version1_0,
},
}
foundOpt = o.GetOneOption(OptionVersion)
require.Equal(t, Version1_1, foundOpt.(Version))
}

0 comments on commit c7b40dc

Please sign in to comment.