diff --git a/dhcpv4/bsdp/bsdp.go b/dhcpv4/bsdp/bsdp.go index 43adcc95..21cc71a6 100644 --- a/dhcpv4/bsdp/bsdp.go +++ b/dhcpv4/bsdp/bsdp.go @@ -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") @@ -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 { @@ -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 } } } @@ -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 } @@ -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, @@ -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 } @@ -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{ @@ -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 } @@ -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 } diff --git a/dhcpv4/bsdp/bsdp_test.go b/dhcpv4/bsdp/bsdp_test.go index cb1a4564..2caa6e50 100644 --- a/dhcpv4/bsdp/bsdp_test.go +++ b/dhcpv4/bsdp/bsdp_test.go @@ -37,7 +37,7 @@ func TestParseBootImageListFromAck(t *testing.T) { }, } ack, _ := dhcpv4.New() - ack.AddOption(&OptVendorSpecificInformation{ + ack.UpdateOption(&OptVendorSpecificInformation{ []dhcpv4.Option{&OptBootImageList{expectedBootImages}}, }) @@ -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") @@ -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 } @@ -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) @@ -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)) @@ -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") @@ -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) @@ -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}) @@ -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}) @@ -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) diff --git a/dhcpv4/bsdp/option_vendor_specific_information.go b/dhcpv4/bsdp/option_vendor_specific_information.go index de144b33..a87135f9 100644 --- a/dhcpv4/bsdp/option_vendor_specific_information.go +++ b/dhcpv4/bsdp/option_vendor_specific_information.go @@ -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) diff --git a/dhcpv4/bsdp/option_vendor_specific_information_test.go b/dhcpv4/bsdp/option_vendor_specific_information_test.go index 64977c4c..ede8a0bf 100644 --- a/dhcpv4/bsdp/option_vendor_specific_information_test.go +++ b/dhcpv4/bsdp/option_vendor_specific_information_test.go @@ -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: - // - // 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{ @@ -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)) } diff --git a/dhcpv4/dhcpv4.go b/dhcpv4/dhcpv4.go index f4e35968..f3700d21 100644 --- a/dhcpv4/dhcpv4.go +++ b/dhcpv4/dhcpv4.go @@ -124,8 +124,6 @@ func New() (*DHCPv4, error) { GatewayIPAddr: net.IPv4zero, Options: make([]Option, 0, 10), } - // the End option has to be added explicitly - d.AddOption(&OptionGeneric{OptionCode: OptionEnd}) return &d, nil } @@ -152,8 +150,8 @@ func NewDiscovery(hwaddr net.HardwareAddr) (*DHCPv4, error) { d.HWType = iana.HWTypeEthernet d.ClientHWAddr = hwaddr d.SetBroadcast() - d.AddOption(&OptMessageType{MessageType: MessageTypeDiscover}) - d.AddOption(&OptParameterRequestList{ + d.UpdateOption(&OptMessageType{MessageType: MessageTypeDiscover}) + d.UpdateOption(&OptParameterRequestList{ RequestedOpts: []OptionCode{ OptionSubnetMask, OptionRouter, @@ -205,7 +203,7 @@ func NewInform(hwaddr net.HardwareAddr, localIP net.IP) (*DHCPv4, error) { d.HWType = iana.HWTypeEthernet d.ClientHWAddr = hwaddr d.ClientIPAddr = localIP - d.AddOption(&OptMessageType{MessageType: MessageTypeInform}) + d.UpdateOption(&OptMessageType{MessageType: MessageTypeInform}) return d, nil } @@ -235,9 +233,9 @@ func NewRequestFromOffer(offer *DHCPv4, modifiers ...Modifier) (*DHCPv4, error) return nil, errors.New("Missing Server IP Address in DHCP Offer") } d.ServerIPAddr = serverIP - d.AddOption(&OptMessageType{MessageType: MessageTypeRequest}) - d.AddOption(&OptRequestedIPAddress{RequestedAddr: offer.YourIPAddr}) - d.AddOption(&OptServerIdentifier{ServerID: serverIP}) + d.UpdateOption(&OptMessageType{MessageType: MessageTypeRequest}) + d.UpdateOption(&OptRequestedIPAddress{RequestedAddr: offer.YourIPAddr}) + d.UpdateOption(&OptServerIdentifier{ServerID: serverIP}) for _, mod := range modifiers { d = mod(d) } @@ -359,16 +357,6 @@ func (d *DHCPv4) SetUnicast() { d.Flags &= ^uint16(0x8000) } -// GetOption will attempt to get all options that match a DHCPv4 option -// from its OptionCode. If the option was not found it will return an -// empty list. -// -// According to RFC 3396, options that are specified more than once are -// concatenated, and hence this should always just return one option. -func (d *DHCPv4) GetOption(code OptionCode) []Option { - return d.Options.Get(code) -} - // GetOneOption will attempt to get an option that match a Option code. // If there are multiple options with the same OptionCode it will only return // the first one found. If no matching option is found nil will be returned. @@ -376,31 +364,10 @@ func (d *DHCPv4) GetOneOption(code OptionCode) Option { return d.Options.GetOne(code) } -// AddOption appends an option to the existing ones. If the last option is an -// OptionEnd, it will be inserted before that. It does not deal with End -// options that appead before the end, like in malformed packets. -func (d *DHCPv4) AddOption(option Option) { - if len(d.Options) == 0 || d.Options[len(d.Options)-1].Code() != OptionEnd { - d.Options = append(d.Options, option) - } else { - end := d.Options[len(d.Options)-1] - d.Options[len(d.Options)-1] = option - d.Options = append(d.Options, end) - } -} - -// UpdateOption updates the existing options with the passed option, adding it +// Update updates the existing options with the passed option, adding it // at the end if not present already func (d *DHCPv4) UpdateOption(option Option) { - for idx, opt := range d.Options { - if opt.Code() == option.Code() { - d.Options[idx] = option - // don't look further - return - } - } - // if not found, add it - d.AddOption(option) + d.Options.Update(option) } // MessageType returns the message type, trying to extract it from the @@ -474,11 +441,13 @@ func (d *DHCPv4) Summary() string { // IsOptionRequested returns true if that option is within the requested // options of the DHCPv4 message. func (d *DHCPv4) IsOptionRequested(requested OptionCode) bool { - for _, optprl := range d.GetOption(OptionParameterRequestList) { - for _, o := range optprl.(*OptParameterRequestList).RequestedOpts { - if o == requested { - return true - } + optprl := d.GetOneOption(OptionParameterRequestList) + if optprl == nil { + return false + } + for _, o := range optprl.(*OptParameterRequestList).RequestedOpts { + if o == requested { + return true } } return false diff --git a/dhcpv4/dhcpv4_test.go b/dhcpv4/dhcpv4_test.go index e5b8ad62..fb0ef702 100644 --- a/dhcpv4/dhcpv4_test.go +++ b/dhcpv4/dhcpv4_test.go @@ -180,64 +180,42 @@ func TestGetOption(t *testing.T) { } hostnameOpt := &OptionGeneric{OptionCode: OptionHostName, Data: []byte("darkstar")} - bootFileOpt1 := &OptBootfileName{"boot.img"} - bootFileOpt2 := &OptBootfileName{"boot2.img"} - d.AddOption(hostnameOpt) - d.AddOption(&OptBootfileName{"boot.img"}) - d.AddOption(&OptBootfileName{"boot2.img"}) - - require.Equal(t, d.GetOption(OptionHostName), []Option{hostnameOpt}) - require.Equal(t, d.GetOption(OptionBootfileName), []Option{bootFileOpt1, bootFileOpt2}) - require.Equal(t, d.GetOption(OptionRouter), []Option{}) + bootFileOpt := &OptBootfileName{"boot.img"} + d.UpdateOption(hostnameOpt) + d.UpdateOption(bootFileOpt) require.Equal(t, d.GetOneOption(OptionHostName), hostnameOpt) - require.Equal(t, d.GetOneOption(OptionBootfileName), bootFileOpt1) + require.Equal(t, d.GetOneOption(OptionBootfileName), bootFileOpt) require.Equal(t, d.GetOneOption(OptionRouter), nil) } -func TestAddOption(t *testing.T) { +func TestUpdateOption(t *testing.T) { d, err := New() require.NoError(t, err) hostnameOpt := &OptionGeneric{OptionCode: OptionHostName, Data: []byte("darkstar")} - bootFileOpt1 := &OptionGeneric{OptionCode: OptionBootfileName, Data: []byte("boot.img")} - bootFileOpt2 := &OptionGeneric{OptionCode: OptionBootfileName, Data: []byte("boot2.img")} - d.AddOption(hostnameOpt) - d.AddOption(bootFileOpt1) - d.AddOption(bootFileOpt2) + bootFileOpt1 := &OptBootfileName{"boot.img"} + bootFileOpt2 := &OptBootfileName{"boot2.img"} + d.UpdateOption(hostnameOpt) + d.UpdateOption(bootFileOpt1) + d.UpdateOption(bootFileOpt2) options := d.Options - require.Equal(t, len(options), 4) - require.Equal(t, options[3].Code(), OptionEnd) -} - -func TestUpdateOption(t *testing.T) { - d, err := New() - require.NoError(t, err) - require.Equal(t, 1, len(d.Options)) - require.Equal(t, OptionEnd, d.Options[0].Code()) - // test that it will add the option since it's missing - d.UpdateOption(&OptDomainName{DomainName: "slackware.it"}) - require.Equal(t, 2, len(d.Options)) - require.Equal(t, OptionDomainName, d.Options[0].Code()) - require.Equal(t, OptionEnd, d.Options[1].Code()) - // test that it won't add another option of the same type - d.UpdateOption(&OptDomainName{DomainName: "slackware.it"}) - require.Equal(t, 2, len(d.Options)) - require.Equal(t, OptionDomainName, d.Options[0].Code()) - require.Equal(t, OptionEnd, d.Options[1].Code()) + require.Equal(t, len(options), 2) + require.Equal(t, d.Options.GetOne(OptionBootfileName), bootFileOpt2) + require.Equal(t, d.Options.GetOne(OptionHostName), hostnameOpt) } func TestDHCPv4NewRequestFromOffer(t *testing.T) { offer, err := New() require.NoError(t, err) offer.SetBroadcast() - offer.AddOption(&OptMessageType{MessageType: MessageTypeOffer}) + offer.UpdateOption(&OptMessageType{MessageType: MessageTypeOffer}) req, err := NewRequestFromOffer(offer) require.Error(t, err) // Now add the option so it doesn't error out. - offer.AddOption(&OptServerIdentifier{ServerID: net.IPv4(192, 168, 0, 1)}) + offer.UpdateOption(&OptServerIdentifier{ServerID: net.IPv4(192, 168, 0, 1)}) // Broadcast request req, err = NewRequestFromOffer(offer) @@ -257,8 +235,8 @@ func TestDHCPv4NewRequestFromOffer(t *testing.T) { func TestDHCPv4NewRequestFromOfferWithModifier(t *testing.T) { offer, err := New() require.NoError(t, err) - offer.AddOption(&OptMessageType{MessageType: MessageTypeOffer}) - offer.AddOption(&OptServerIdentifier{ServerID: net.IPv4(192, 168, 0, 1)}) + offer.UpdateOption(&OptMessageType{MessageType: MessageTypeOffer}) + offer.UpdateOption(&OptServerIdentifier{ServerID: net.IPv4(192, 168, 0, 1)}) userClass := WithUserClass([]byte("linuxboot"), false) req, err := NewRequestFromOffer(offer, userClass) require.NoError(t, err) @@ -306,7 +284,6 @@ func TestNewDiscovery(t *testing.T) { require.Equal(t, hwAddr, m.ClientHWAddr) require.True(t, m.IsBroadcast()) require.True(t, m.Options.Has(OptionParameterRequestList)) - require.True(t, m.Options.Has(OptionEnd)) } func TestNewInform(t *testing.T) { @@ -328,7 +305,7 @@ func TestIsOptionRequested(t *testing.T) { require.False(t, pkt.IsOptionRequested(OptionDomainNameServer)) optprl := OptParameterRequestList{RequestedOpts: []OptionCode{OptionDomainNameServer}} - pkt.AddOption(&optprl) + pkt.UpdateOption(&optprl) require.True(t, pkt.IsOptionRequested(OptionDomainNameServer)) } diff --git a/dhcpv4/modifiers.go b/dhcpv4/modifiers.go index db673039..e7f6576f 100644 --- a/dhcpv4/modifiers.go +++ b/dhcpv4/modifiers.go @@ -37,7 +37,7 @@ func WithHwAddr(hwaddr net.HardwareAddr) Modifier { // WithOption appends a DHCPv4 option provided by the user func WithOption(opt Option) Modifier { return func(d *DHCPv4) *DHCPv4 { - d.AddOption(opt) + d.UpdateOption(opt) return d } } @@ -52,7 +52,7 @@ func WithUserClass(uc []byte, rfc bool) Modifier { UserClasses: [][]byte{uc}, Rfc3004: rfc, } - d.AddOption(&ouc) + d.UpdateOption(&ouc) return d } } @@ -85,7 +85,7 @@ func WithNetboot(d *DHCPv4) *DHCPv4 { OptParams = &OptParameterRequestList{ RequestedOpts: []OptionCode{OptionTFTPServerName, OptionBootfileName}, } - d.AddOption(OptParams) + d.UpdateOption(OptParams) } return d } @@ -96,7 +96,7 @@ func WithRequestedOptions(optionCodes ...OptionCode) Modifier { params := d.GetOneOption(OptionParameterRequestList) if params == nil { params = &OptParameterRequestList{} - d.AddOption(params) + d.UpdateOption(params) } opts := params.(*OptParameterRequestList) for _, optionCode := range optionCodes { diff --git a/dhcpv4/modifiers_test.go b/dhcpv4/modifiers_test.go index 6d3976e2..e6538178 100644 --- a/dhcpv4/modifiers_test.go +++ b/dhcpv4/modifiers_test.go @@ -77,7 +77,7 @@ func TestWithNetbootExistingTFTP(t *testing.T) { OptParams := &OptParameterRequestList{ RequestedOpts: []OptionCode{OptionTFTPServerName}, } - d.AddOption(OptParams) + d.UpdateOption(OptParams) d = WithNetboot(d) require.Equal(t, "Parameter Request List -> [TFTP Server Name, Bootfile Name]", d.Options[0].String()) } @@ -87,7 +87,7 @@ func TestWithNetbootExistingBootfileName(t *testing.T) { OptParams := &OptParameterRequestList{ RequestedOpts: []OptionCode{OptionBootfileName}, } - d.AddOption(OptParams) + d.UpdateOption(OptParams) d = WithNetboot(d) require.Equal(t, "Parameter Request List -> [Bootfile Name, TFTP Server Name]", d.Options[0].String()) } @@ -97,7 +97,7 @@ func TestWithNetbootExistingBoth(t *testing.T) { OptParams := &OptParameterRequestList{ RequestedOpts: []OptionCode{OptionBootfileName, OptionTFTPServerName}, } - d.AddOption(OptParams) + d.UpdateOption(OptParams) d = WithNetboot(d) require.Equal(t, "Parameter Request List -> [Bootfile Name, TFTP Server Name]", d.Options[0].String()) } diff --git a/dhcpv4/options.go b/dhcpv4/options.go index 44bf1d3f..a3427ce3 100644 --- a/dhcpv4/options.go +++ b/dhcpv4/options.go @@ -95,21 +95,6 @@ func ParseOption(code OptionCode, data []byte) (Option, error) { // Options is a collection of options. type Options []Option -// Get will attempt to get all options that match a DHCPv4 option from its -// OptionCode. If the option was not found it will return an empty list. -// -// According to RFC 3396, options that are specified more than once are -// concatenated, and hence this should always just return one option. -func (o Options) Get(code OptionCode) []Option { - opts := []Option{} - for _, opt := range o { - if opt.Code() == code { - opts = append(opts, opt) - } - } - return opts -} - // GetOne will attempt to get an option that match a Option code. If there // are multiple options with the same OptionCode it will only return the first // one found. If no matching option is found nil will be returned. @@ -127,6 +112,34 @@ func (o Options) Has(code OptionCode) bool { return o.GetOne(code) != nil } +// Add appends an option to the existing ones. +// +// An End option is ignored, as it is inserted by the marshaling code anyway. +func (o *Options) Add(option Option) { +} + +// Update updates the first existing options with the passed option, adding it +// at the end if not present already. +// +// RFC 2131, Section 4.1 "Options may appear only once." +// +// An End option is ignored. +func (o *Options) Update(option Option) { + if option.Code() == OptionEnd { + return + } + + for idx, opt := range *o { + if opt.Code() == option.Code() { + (*o)[idx] = option + // Don't look further. + return + } + } + // If not found, add it. + *o = append(*o, option) +} + // OptionsFromBytes parses a sequence of bytes until the end and builds a list // of options from it. // @@ -187,8 +200,14 @@ func OptionsFromBytesWithParser(data []byte, coder OptionCodeGetter, parser Opti if _, ok := options[c]; !ok { order = append(order, c) } - // RFC 3396: Just concatenate the data if the option code was - // specified multiple times. + + // RFC 2131, Section 4.1 "Options may appear only once, [...]. + // The client concatenates the values of multiple instances of + // the same option into a single parameter list for + // configuration." + // + // See also RFC 3396 for concatenation order and options longer + // than 255 bytes. options[c] = append(options[c], data...) } diff --git a/dhcpv4/server_test.go b/dhcpv4/server_test.go index bb2ee11c..4307924c 100644 --- a/dhcpv4/server_test.go +++ b/dhcpv4/server_test.go @@ -41,7 +41,7 @@ func DORAHandler(conn net.PacketConn, peer net.Addr, m *DHCPv4) { log.Printf("NewReplyFromRequest failed: %v", err) return } - reply.AddOption(&OptServerIdentifier{ServerID: net.IP{1, 2, 3, 4}}) + reply.UpdateOption(&OptServerIdentifier{ServerID: net.IP{1, 2, 3, 4}}) opt := m.GetOneOption(OptionDHCPMessageType) if opt == nil { log.Printf("No message type found!") @@ -49,9 +49,9 @@ func DORAHandler(conn net.PacketConn, peer net.Addr, m *DHCPv4) { } switch opt.(*OptMessageType).MessageType { case MessageTypeDiscover: - reply.AddOption(&OptMessageType{MessageType: MessageTypeOffer}) + reply.UpdateOption(&OptMessageType{MessageType: MessageTypeOffer}) case MessageTypeRequest: - reply.AddOption(&OptMessageType{MessageType: MessageTypeAck}) + reply.UpdateOption(&OptMessageType{MessageType: MessageTypeAck}) default: log.Printf("Unhandled message type: %v", opt.(*OptMessageType).MessageType) return