From: Jack Burton Subject: [patch 3 of 3] iscsid(8): support a wider variety of iscsi targets To: tech@openbsd.org Date: Tue, 31 Dec 2024 18:51:39 +1030 This patch allows iscsid(8) to connect successfully to & use a wider variety of iscsi targets. It solves three current issues and makes a start on a fourth one: 1. Some common iscsi targets refuse to complete SECNEG if we do not send a SessionType proposal, even for normal sessions (which according to RFC7143 is supposed to be the default). Currently iscsid(8) only sends a SessionType proposal for discovery sessions. 2. Some common iscsi targets complain if we send the same proposal more than once during login (in SECNEG and/or OPNEG). Currently, if iscsid(8) sends MaxConnections or MaxRecvDataSegmentLength at all then it keeps sending them over & over again throughout OPNEG until the login phase succeeds or fails (but if they're not accepted the first time, it will always end up failing). 3. RFC7143 requires a response to every (non-declarative) proposal and some common iscsi targets enforce that requirement. Most proposals can be made either by the initiator or by the target. Since iscsid(8) currently only sends between 0 and 2 proposals in OPNEG, many common iscsi targets end up sending proposals to us, which we don't handle. 4. iscsid(8) currently ignores {Header,Data}Digest proposals. This patch does *not* implement CRC digests in the full feature phase (can do that later if need be), but it does at least handle *Digest proposals more gracefully during the login phase while still ensuring we negotiate *Digest=None so we don't break FFP. #3 above is the main focus of this patch, but it made sense to deal with the other 3 issues at the same time as they're all inter-related. My approach is simple: since both parties can make proposals during the login phase, simply seize the opportunity to "go first", sending proposals for everything that we're allowed to at any given time, following the rules in Section 13 of RFC7143. This approach ensures that every connection attempt either succeeds or fails after sending only 3 PDUs, of which only 2 (1 each in SECNEG & OPNEG) contain any proposals. That is much simpler than the other obvious alternative, which would be to keep track of (and continually check!) the history of a series of proposals & responses throughout a login phase potentially consisting of more proposal/response cycles (without knowing in advance which keys will be proposed first by each party). [simpler still would be to insist on going straight to OPNEG, which RFC7143 allows, but that would break if we want to support any non-None AuthMethod in the future] Since the standard provides many optional knobs to twiddle and we don't expose those to user config (nor should we need to!), the simple approach is sufficient (at least for all the targets I've tested with: more test reports would be welcome!). It is also necessary, to address to the issues described above. Also noted the updated/consolidated RFC in the man page. NOTE: this patch replaces patch #1 in this set (it includes all of patch #1 except the bits it obsoletes), but it is independent of patch #2 in the set (which can be applied cleanly on its own, with or without this one). I posted them as 3 separate patches here for ease of review as they each address logically different issues (and #3 introduces new functionality, whereas #1 & #2 are straightforward bug fixes). Reviews / comments / criticisms / test reports welcome. cvs server: Diffing . Index: connection.c =================================================================== RCS file: /cvs/src/usr.sbin/iscsid/connection.c,v diff -u -p -r1.21 connection.c --- connection.c 5 Dec 2015 06:38:18 -0000 1.21 +++ connection.c 31 Dec 2024 07:36:53 -0000 @@ -279,6 +279,19 @@ log_debug("SET_BOOL: %s = %u", #v, (int) } \ } while (0) +#define SET_DIGEST(p, x, v) \ +do { \ + if (!strcmp((p)->key, #v)) { \ + (x)->his.v = text_to_digest((p)->value, &err); \ + if (err) { \ + log_warnx("bad param %s=%s: %s", \ + (p)->key, (p)->value, err); \ + errors++; \ + } \ +log_debug("SET_DIGEST: %s = %u", #v, (u_int8_t)(x)->his.v); \ + } \ +} while (0) + int conn_parse_kvp(struct connection *c, struct kvp *kvp) { @@ -291,6 +304,8 @@ conn_parse_kvp(struct connection *c, str for (k = kvp; k->key; k++) { log_debug("conn_parse_kvp: %s = %s", k->key, k->value); /* XXX handle NotUnderstood|Irrelevant|Reject */ + SET_DIGEST(k, c, HeaderDigest); + SET_DIGEST(k, c, DataDigest); SET_NUM(k, s, MaxBurstLength, 512, 16777215); SET_NUM(k, s, FirstBurstLength, 512, 16777215); SET_NUM(k, s, DefaultTime2Wait, 0, 3600); @@ -315,44 +330,104 @@ log_debug("conn_parse_kvp: %s = %s", k-> #undef SET_NUM #undef SET_BOOL +#undef SET_DIGEST + +#define GET_BOOL_P(dst, req, k, src, f) \ +do { \ + if (f == 0 && !strcmp(req, #k)) { \ + (dst)->value = ((src)->mine.k) ? "Yes" : "No"; \ + f++; \ + } \ +} while (0) + +#define GET_DIGEST_P(dst, req, k, src, f) \ +do { \ + if (f == 0 && !strcmp(req, #k)) { \ + (dst)->value = \ + ((src)->mine.k == DIGEST_NONE) ? "None" : "CRC32C"; \ + f++; \ + } \ +} while (0) + +#define GET_NUM_P(dst, req, k, src, f, e) \ +do { \ + if (f == 0 && !strcmp(req, #k)) { \ + if (asprintf(&((dst)->value), "%u", (src)->mine.k) == -1)\ + e++; \ + else \ + (dst)->flags |= KVP_VALUE_ALLOCED; \ + f++; \ + } \ +} while (0) + +#define GET_STR_C(dst, req, k, src, f) \ +do { \ + if (f == 0 && !strcmp(req, #k)) { \ + (dst)->value = (src)->config.k; \ + f++; \ + } \ +} while (0) + +#define GET_STYPE_C(dst, req, k, src, f) \ +do { \ + if (f == 0 && !strcmp(req, #k)) { \ + (dst)->value = ((src)->config.k == SESSION_TYPE_DISCOVERY)\ + ? "Discovery" : "Normal"; \ + f++; \ + } \ +} while (0) int -conn_gen_kvp(struct connection *c, struct kvp *kvp, size_t *nkvp) -{ - struct session *s = c->session; - size_t i = 0; +kvp_set_from_mine(struct kvp *kvp, const char *key, struct connection *c) { + int e = 0, f = 0; - if (s->mine.MaxConnections != iscsi_sess_defaults.MaxConnections) { - if (kvp && i < *nkvp) { - kvp[i].key = strdup("MaxConnections"); - if (kvp[i].key == NULL) - return -1; - if (asprintf(&kvp[i].value, "%hu", - s->mine.MaxConnections) == -1) { - kvp[i].value = NULL; - return -1; - } - } - i++; + if ((kvp->flags & KVP_KEY_ALLOCED)) + free(kvp->key); + kvp->key = NULL; + if ((kvp->flags & KVP_VALUE_ALLOCED)) + free(kvp->value); + kvp->value = NULL; + + if ((kvp->key = strdup(key)) == NULL) + return 1; + kvp->flags = KVP_KEY_ALLOCED; + + /* XXX handle at least CHAP */ + if (!strcmp(key, "AuthMethod")) { + kvp->value = "None"; + return 0; } - if (c->mine.MaxRecvDataSegmentLength != - iscsi_conn_defaults.MaxRecvDataSegmentLength) { - if (kvp && i < *nkvp) { - kvp[i].key = strdup("MaxRecvDataSegmentLength"); - if (kvp[i].key == NULL) - return -1; - if (asprintf(&kvp[i].value, "%u", - c->mine.MaxRecvDataSegmentLength) == -1) { - kvp[i].value = NULL; - return -1; - } - } - i++; + GET_DIGEST_P(kvp, key, HeaderDigest, c, f); + GET_DIGEST_P(kvp, key, DataDigest, c, f); + GET_NUM_P(kvp, key, MaxConnections, c->session, f, e); + GET_STR_C(kvp, key, TargetName, c->session, f); + GET_STR_C(kvp, key, InitiatorName, c->session, f); + GET_BOOL_P(kvp, key, InitialR2T, c->session, f); + GET_BOOL_P(kvp, key, ImmediateData, c->session, f); + GET_NUM_P(kvp, key, MaxRecvDataSegmentLength, c, f, e); + GET_NUM_P(kvp, key, MaxBurstLength, c->session, f, e); + GET_NUM_P(kvp, key, FirstBurstLength, c->session, f, e); + GET_NUM_P(kvp, key, DefaultTime2Wait, c->session, f, e); + GET_NUM_P(kvp, key, DefaultTime2Retain, c->session, f, e); + GET_NUM_P(kvp, key, MaxOutstandingR2T, c->session, f, e); + GET_BOOL_P(kvp, key, DataPDUInOrder, c->session, f); + GET_BOOL_P(kvp, key, DataSequenceInOrder, c->session, f); + GET_NUM_P(kvp, key, ErrorRecoveryLevel, c->session, f, e); + GET_STYPE_C(kvp, key, SessionType, c->session, f); + /* XXX handle TaskReporting */ + + if (f == 0) { + errno = EINVAL; + return 1; } - - *nkvp = i; - return 0; + return e; } + +#undef GET_BOOL_P +#undef GET_DIGEST_P +#undef GET_NUM_P +#undef GET_STR_C +#undef GET_STYPE_C void conn_pdu_write(struct connection *c, struct pdu *p) Index: initiator.c =================================================================== RCS file: /cvs/src/usr.sbin/iscsid/initiator.c,v diff -u -p -r1.15 initiator.c --- initiator.c 16 Jan 2015 15:57:06 -0000 1.15 +++ initiator.c 31 Dec 2024 07:36:53 -0000 @@ -48,6 +48,7 @@ struct task_logout { u_int8_t reason; }; +int conn_is_leading(struct connection *); struct kvp *initiator_login_kvp(struct connection *, u_int8_t); struct pdu *initiator_login_build(struct connection *, struct task_login *); @@ -250,47 +251,74 @@ initiator_nop_in_imm(struct connection * conn_task_issue(c, t); } +int +conn_is_leading(struct connection *c) +{ + struct connection *lead; + + if (c == NULL || c->session == NULL + || (lead = TAILQ_FIRST(&c->session->connections)) == NULL) + return 0; + return (c == lead) ? 1 : 0; +} + +#define MINE_NOT_DEFAULT(c, k) ((c)->mine.k != iscsi_conn_defaults.k) + struct kvp * initiator_login_kvp(struct connection *c, u_int8_t stage) { - struct kvp *kvp; - size_t nkvp; + struct kvp *kvp = NULL; + size_t i = 0, len; + const char *leading_only[] = {"MaxConnections", "InitialR2T", + "ImmediateData", "MaxBurstLength", "FirstBurstLength", + "DefaultTime2Wait", "DefaultTime2Retain", "MaxOutstandingR2T", + "DataPDUInOrder", "DataSequenceInOrder", "ErrorRecoveryLevel", + NULL}; + const char *opneg_always[] = {"HeaderDigest", "DataDigest", NULL}; + const char *secneg[] = {"SessionType", "InitiatorName", "TargetName", + "AuthMethod", NULL}; + const char **p; switch (stage) { case ISCSI_LOGIN_STG_SECNEG: - if (!(kvp = calloc(4, sizeof(*kvp)))) + len = sizeof(secneg) / sizeof(*secneg); + if (!(kvp = calloc(len + 1, sizeof(*kvp)))) return NULL; - kvp[0].key = "AuthMethod"; - kvp[0].value = "None"; - kvp[1].key = "InitiatorName"; - kvp[1].value = c->session->config.InitiatorName; - - if (c->session->config.SessionType == SESSION_TYPE_DISCOVERY) { - kvp[2].key = "SessionType"; - kvp[2].value = "Discovery"; - } else { - kvp[2].key = "TargetName"; - kvp[2].value = c->session->config.TargetName; - } + for (p = secneg; *p != NULL; i++, p++) + if (kvp_set_from_mine(&kvp[i], *p, c)) + goto fail; break; case ISCSI_LOGIN_STG_OPNEG: - if (conn_gen_kvp(c, NULL, &nkvp) == -1) - return NULL; - nkvp += 1; /* add slot for terminator */ - if (!(kvp = calloc(nkvp, sizeof(*kvp)))) + len = sizeof(opneg_always) / sizeof(*opneg_always); + if (conn_is_leading(c)) + len += sizeof(leading_only) / sizeof(*leading_only); + if (MINE_NOT_DEFAULT(c, MaxRecvDataSegmentLength)) + len++; + if (!(kvp = calloc(len + 1, sizeof(*kvp)))) return NULL; - if (conn_gen_kvp(c, kvp, &nkvp) == -1) { - free(kvp); - return NULL; - } + for (p = opneg_always; *p != NULL; i++, p++) + if (kvp_set_from_mine(&kvp[i], *p, c)) + goto fail; + if (conn_is_leading(c)) + for (p = leading_only; *p != NULL; i++, p++) + if (kvp_set_from_mine(&kvp[i], *p, c)) + goto fail; + if (MINE_NOT_DEFAULT(c, MaxRecvDataSegmentLength) && + kvp_set_from_mine(&kvp[i], "MaxRecvDataSegmentLength", c)) + goto fail; break; default: log_warnx("initiator_login_kvp: exit stage left"); return NULL; } return kvp; +fail: + kvp_free(kvp); + return NULL; } +#undef MINE_NOT_DEFAULT + struct pdu * initiator_login_build(struct connection *c, struct task_login *tl) { @@ -327,10 +355,10 @@ initiator_login_build(struct connection return NULL; } if ((n = text_to_pdu(kvp, p)) == -1) { - free(kvp); + kvp_free(kvp); return NULL; } - free(kvp); + kvp_free(kvp); if (n > 8192) { log_warn("initiator_login_build: help, I'm too verbose"); @@ -409,11 +437,11 @@ initiator_login_cb(struct connection *c, } if (conn_parse_kvp(c, kvp) == -1) { - free(kvp); + kvp_free(kvp); conn_fail(c); goto done; } - free(kvp); + kvp_free(kvp); } /* advance FSM if possible */ @@ -480,7 +508,7 @@ initiator_discovery_cb(struct connection for (k = kvp; k->key; k++) { log_debug("%s\t=>\t%s", k->key, k->value); } - free(kvp); + kvp_free(kvp); session_shutdown(c->session); break; default: Index: iscsid.8 =================================================================== RCS file: /cvs/src/usr.sbin/iscsid/iscsid.8,v diff -u -p -r1.10 iscsid.8 --- iscsid.8 27 Jul 2015 17:28:39 -0000 1.10 +++ iscsid.8 31 Dec 2024 07:36:53 -0000 @@ -74,14 +74,13 @@ socket used for communication with .Xr iscsictl 8 .Sh STANDARDS .Rs +.%A M. Chadalapaka .%A J. Satran .%A K. Meth -.%A C. Sapuntzakis -.%A M. Chadalapaka -.%A E. Zeidner -.%D April 2004 -.%R RFC 3720 -.%T Internet Small Computer Systems Interface (iSCSI) +.%A D. Black +.%D April 2014 +.%R RFC 7143 +.%T Internet Small Computer Systems Interface (iSCSI) Protocol (Consolidated) .Re .Pp .Rs Index: iscsid.c =================================================================== RCS file: /cvs/src/usr.sbin/iscsid/iscsid.c,v diff -u -p -r1.22 iscsid.c --- iscsid.c 16 Apr 2021 14:37:06 -0000 1.22 +++ iscsid.c 31 Dec 2024 07:36:53 -0000 @@ -58,6 +58,8 @@ const struct session_params iscsi_sess_d }; const struct connection_params iscsi_conn_defaults = { + .HeaderDigest = DIGEST_NONE, + .DataDigest = DIGEST_NONE, .MaxRecvDataSegmentLength = 8192 }; @@ -354,7 +356,8 @@ iscsi_merge_conn_params(struct connectio struct connection_params *mine, struct connection_params *his) { res->MaxRecvDataSegmentLength = his->MaxRecvDataSegmentLength; - /* XXX HeaderDigest and DataDigest */ + MERGE_MIN(res, mine, his, HeaderDigest); + MERGE_MIN(res, mine, his, DataDigest); } #undef MERGE_MIN Index: iscsid.h =================================================================== RCS file: /cvs/src/usr.sbin/iscsid/iscsid.h,v diff -u -p -r1.19 iscsid.h --- iscsid.h 21 May 2024 05:00:48 -0000 1.19 +++ iscsid.h 31 Dec 2024 07:36:53 -0000 @@ -181,6 +181,8 @@ struct session_config { u_int8_t disabled; }; +#define DIGEST_NONE 1 +#define DIGEST_CRC32C 2 #define SESSION_TYPE_NORMAL 0 #define SESSION_TYPE_DISCOVERY 1 @@ -359,7 +361,7 @@ void conn_task_issue(struct connection * void conn_task_schedule(struct connection *); void conn_task_cleanup(struct connection *, struct task *); int conn_parse_kvp(struct connection *, struct kvp *); -int conn_gen_kvp(struct connection *, struct kvp *, size_t *); +int kvp_set_from_mine(struct kvp *, const char *, struct connection *); void conn_pdu_write(struct connection *, struct pdu *); void conn_fail(struct connection *); void conn_fsm(struct connection *, enum c_event); @@ -369,6 +371,7 @@ int text_to_pdu(struct kvp *, struct pdu struct kvp *pdu_to_text(char *, size_t); u_int64_t text_to_num(const char *, u_int64_t, u_int64_t, const char **); int text_to_bool(const char *, const char **); +u_int8_t text_to_digest(const char *, const char **); void pdu_free_queue(struct pduq *); ssize_t pdu_read(struct connection *); ssize_t pdu_write(struct connection *); @@ -385,6 +388,7 @@ void *pdu_getbuf(struct pdu *, size_t *, void pdu_free(struct pdu *); int socket_setblockmode(int, int); const char *log_sockaddr(void *); +void kvp_free(struct kvp *); void task_init(struct task *, struct session *, int, void *, void (*)(struct connection *, void *, struct pdu *), Index: pdu.c =================================================================== RCS file: /cvs/src/usr.sbin/iscsid/pdu.c,v diff -u -p -r1.13 pdu.c --- pdu.c 12 Apr 2021 10:03:33 -0000 1.13 +++ pdu.c 31 Dec 2024 07:36:53 -0000 @@ -210,6 +210,32 @@ text_to_bool(const char *buf, const char return val; } +u_int8_t +text_to_digest(const char *buf, const char **errstrp) +{ + int e = 0; + size_t len; + char *p; + u_int8_t val = 0; + + while (buf != NULL) { + p = strchr(buf, ','); + len = (p == NULL) ? strlen(buf) : p - buf; + if (!strncmp(buf, "None", len)) + val |= DIGEST_NONE; + else if (!strncmp(buf, "CRC32C", len)) + val |= DIGEST_CRC32C; + else + e++; + buf = (p == NULL) ? NULL : p + 1; + } + + if (e != 0) + val = 0; + if (errstrp != NULL) + *errstrp = (e == 0) ? NULL : "invalid"; + return val; +} /* * Internal functions to send/recv pdus. Index: session.c =================================================================== RCS file: /cvs/src/usr.sbin/iscsid/session.c,v diff -u -p -r1.9 session.c --- session.c 8 Mar 2023 04:43:13 -0000 1.9 +++ session.c 31 Dec 2024 07:36:53 -0000 @@ -144,6 +144,10 @@ session_config(struct session *s, struct fatal("strdup"); } else s->config.InitiatorName = default_initiator_name(); + if (!s->config.HeaderDigest) + s->config.HeaderDigest = DIGEST_NONE; + if (!s->config.DataDigest) + s->config.DataDigest = DIGEST_NONE; } void Index: util.c =================================================================== RCS file: /cvs/src/usr.sbin/iscsid/util.c,v diff -u -p -r1.9 util.c --- util.c 12 Apr 2021 10:03:33 -0000 1.9 +++ util.c 31 Dec 2024 07:36:53 -0000 @@ -189,3 +189,19 @@ fail: pdu_free(pdu); return -1; } + +void +kvp_free(struct kvp *kvp) +{ + struct kvp *k; + + if (kvp == NULL) + return; + for (k = kvp; k->key; k++) { + if (k->flags & KVP_KEY_ALLOCED) + free(k->key); + if (k->flags & KVP_VALUE_ALLOCED) + free(k->value); + } + free(kvp); +}