Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix using multiple enterprise IDs with vendclass (Option 124 DHCP / Option 16 DHCPv6) (#328) #408

Merged
merged 5 commits into from
Jan 18, 2025
Merged

Fix using multiple enterprise IDs with vendclass (Option 124 DHCP / Option 16 DHCPv6) (#328) #408

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c0a58f4
Fix using multiple enterprise IDs (Option 124 DHCP / 17 DHCP6) (#328)
spoljak-ent Nov 21, 2024
bcf800c
Remove comment
spoljak-ent Nov 21, 2024
c68cea4
Required fixes
spoljak-ent Nov 22, 2024
1f3cd2d
added 4 spaces on line breaks
spoljak-ent Nov 22, 2024
d7075fe
forgot to remove the space after the comma :(
spoljak-ent Nov 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 22 additions & 19 deletions src/dhcp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -808,7 +808,7 @@ make_message(struct bootp **bootpm, const struct interface *ifp, uint8_t type)
const struct dhcp_lease *lease = &state->lease;
char hbuf[HOSTNAME_MAX_LEN + 1];
const char *hostname;
const struct vivco *vivco;
const struct vivco *vivco, *vivco_endp = ifo->vivco + ifo->vivco_len;;
int mtu;
#ifdef AUTH
uint8_t *auth, auth_len;
Expand Down Expand Up @@ -1142,26 +1142,29 @@ make_message(struct bootp **bootpm, const struct interface *ifp, uint8_t type)
{
AREA_CHECK(sizeof(ul));
*p++ = DHO_VIVCO;
lp = p++;
*lp = sizeof(ul);
ul = htonl(ifo->vivco_en);
memcpy(p, &ul, sizeof(ul));
p += sizeof(ul);
for (i = 0, vivco = ifo->vivco;
i < ifo->vivco_len;
i++, vivco++)
{
AREA_FIT(vivco->len);
if (vivco->len + 2 + *lp > 255) {
logerrx("%s: VIVCO option too big",
ifp->name);
free(bootp);
return -1;
}
*p++ = (uint8_t)vivco->len;
size_t totallen = 0;
uint8_t datalen, datalenopt;
for (vivco = ifo->vivco; vivco != vivco_endp; vivco++)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

White space between the declaration and the code please, but see below comment first.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved with RFC3396 ctx.

totallen += sizeof(uint32_t) + 2 * sizeof(uint8_t) + vivco->len;
if (totallen > UINT8_MAX) {
logerrx("%s: VIVCO option too big",
ifp->name);
free(bootp);
return -1;
}
*p++ = (uint8_t)totallen;
for (vivco = ifo->vivco; vivco != vivco_endp; vivco++) {
ul = htonl(vivco->en);
memcpy(p, &ul, sizeof(ul));
p += sizeof(ul);
datalen = (uint8_t)(sizeof(uint8_t) + vivco->len);
memcpy(p, &datalen, sizeof(datalen));
p += sizeof(datalen);
datalenopt = (uint8_t)(vivco->len);
memcpy(p, &datalenopt, sizeof(datalenopt));
p += sizeof(datalenopt);
memcpy(p, vivco->data, vivco->len);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be re-written as

*p++ = (uint8_t)(sizeof(uint8_t) + vivco->len);
*p++ = (uint8_t)(vivco->len);

Much smaller code and no need to declare the variables then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I'll fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made the requested change with RFC3396 ctx.
Looks much cleaner now.

p += vivco->len;
*lp = (uint8_t)(*lp + vivco->len + 1);
}
}

Expand Down
41 changes: 22 additions & 19 deletions src/dhcp6.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,25 +280,22 @@ static size_t
dhcp6_makevendor(void *data, const struct interface *ifp)
{
const struct if_options *ifo;
size_t len, vlen, i;
size_t len = 0, optlen, vlen, i;
uint8_t *p;
const struct vivco *vivco;
struct dhcp6_option o;

ifo = ifp->options;
len = sizeof(uint32_t); /* IANA PEN */
if (ifo->vivco_en) {
vlen = 0;
if (ifo->vivco_len > 0) {
for (i = 0, vivco = ifo->vivco;
i < ifo->vivco_len;
i++, vivco++)
vlen += sizeof(uint16_t) + vivco->len;
len += vlen;
len += sizeof(o) + sizeof(uint32_t) + sizeof(uint16_t) + vivco->len;
} else if (ifo->vendorclassid[0] != '\0') {
/* dhcpcd owns DHCPCD_IANA_PEN.
* If you need your own string, get your own IANA PEN. */
vlen = strlen(ifp->ctx->vendor);
len += sizeof(uint16_t) + vlen;
len += sizeof(o) + sizeof(uint32_t) + sizeof(uint16_t) + vlen;
} else
return 0;

Expand All @@ -312,34 +309,40 @@ dhcp6_makevendor(void *data, const struct interface *ifp)
uint16_t hvlen;

p = data;
o.code = htons(D6_OPTION_VENDOR_CLASS);
o.len = htons((uint16_t)len);
memcpy(p, &o, sizeof(o));
p += sizeof(o);
pen = htonl(ifo->vivco_en ? ifo->vivco_en : DHCPCD_IANA_PEN);
memcpy(p, &pen, sizeof(pen));
p += sizeof(pen);

if (ifo->vivco_en) {
if (ifo->vivco_len > 0) {
for (i = 0, vivco = ifo->vivco;
i < ifo->vivco_len;
i++, vivco++)
{
i++, vivco++) {
optlen = sizeof(uint32_t) + sizeof(uint16_t) + vivco->len;
o.code = htons(D6_OPTION_VENDOR_CLASS);
o.len = htons((uint16_t)optlen);
memcpy(p, &o, sizeof(o));
p += sizeof(o);
pen = htonl(vivco->en);
memcpy(p, &pen, sizeof(pen));
p += sizeof(pen);
hvlen = htons((uint16_t)vivco->len);
memcpy(p, &hvlen, sizeof(hvlen));
p += sizeof(hvlen);
memcpy(p, vivco->data, vivco->len);
p += vivco->len;
}
} else if (ifo->vendorclassid[0] != '\0') {
o.code = htons(D6_OPTION_VENDOR_CLASS);
o.len = htons((uint16_t)len);
memcpy(p, &o, sizeof(o));
p += sizeof(o);
pen = htonl(DHCPCD_IANA_PEN);
memcpy(p, &pen, sizeof(pen));
p += sizeof(pen);
hvlen = htons((uint16_t)vlen);
memcpy(p, &hvlen, sizeof(hvlen));
p += sizeof(hvlen);
memcpy(p, ifp->ctx->vendor, vlen);
}
}

return sizeof(o) + len;
return len;
}

#ifndef SMALL
Expand Down
9 changes: 8 additions & 1 deletion src/if-options.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -656,6 +656,7 @@ parse_option(struct dhcpcd_ctx *ctx, const char *ifname, struct if_options *ifo,
struct dhcp_opt **dop, *ndop;
size_t *dop_len, dl, odl;
struct vivco *vivco;
const struct vivco *vivco_endp = ifo->vivco + ifo->vivco_len;
struct group *grp;
#ifdef AUTH
struct token *token;
Expand Down Expand Up @@ -2119,6 +2120,12 @@ parse_option(struct dhcpcd_ctx *ctx, const char *ifname, struct if_options *ifo,
logerrx("invalid code: %s", arg);
return -1;
}
for (vivco = ifo->vivco; vivco != vivco_endp; vivco++) {
if (vivco->en == (uint32_t)u) {
logerrx("only one vendor class option per enterprise number");
return -1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a better text would be "vendor class option for enterprise number %u already defined".
We might also want to consider do we want to have a different option sent for the same enterprise number based on interface or profile name? If so, we might just want to silently override it instead.

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it is better regarding the text.
Regarding the different option, I was going along with RFC 3925 that we should have only one enterprise number among all instances of this option.
How would this work with the silent override?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would just free what was there and put new data in?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the confusion, I meant about having a different option sent for the same enterprise number based on interface or profile name. Haven't worked with multiple profiles/interfaces tbh.

}
}
fp = strskipwhite(fp);
if (fp) {
s = parse_string(NULL, 0, fp);
Expand Down Expand Up @@ -2149,8 +2156,8 @@ parse_option(struct dhcpcd_ctx *ctx, const char *ifname, struct if_options *ifo,
return -1;
}
ifo->vivco = vivco;
ifo->vivco_en = (uint32_t)u;
vivco = &ifo->vivco[ifo->vivco_len++];
vivco->en = (uint32_t)u;
vivco->len = dl;
vivco->data = (uint8_t *)np;
break;
Expand Down
3 changes: 1 addition & 2 deletions src/if-options.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@
#define USERCLASS_MAX_LEN 255
#define VENDOR_MAX_LEN 255
#define MUDURL_MAX_LEN 255
#define ENTERPRISE_NUMS_MAX_LEN 255

#define DHCPCD_ARP (1ULL << 0)
#define DHCPCD_RELEASE (1ULL << 1)
Expand Down Expand Up @@ -221,6 +220,7 @@ struct if_ia {
};

struct vivco {
uint32_t en;
size_t len;
uint8_t *data;
};
Expand Down Expand Up @@ -303,7 +303,6 @@ struct if_options {
size_t nd_override_len;
struct dhcp_opt *dhcp6_override;
size_t dhcp6_override_len;
uint32_t vivco_en;
struct vivco *vivco;
size_t vivco_len;
struct dhcp_opt *vivso_override;
Expand Down
Loading