Vk 2052 introduce ja5t calc #2289
Conversation
…esta-tech/tempesta into vk-2052-introduce-ja5t-calc
| @@ -545,7 +545,7 @@ tfw_connection_link_to_sk(TfwConn *conn, struct sock *sk) | |||
| } | |||
|
|
|||
| /* | |||
| * Do an opposite to what tfw_connection_link_from_sk() does. | |||
| * Do an opposite to what tfw_connection_link_to_sk() does. | |||
There was a problem hiding this comment.
Yeah, the to and from naming are confusing, but this function nulifies sk->sk_user_data set in tfw_connection_link_from_sk(), so it seems the original comment is correct
| @@ -269,6 +269,34 @@ struct ttls_alpn_proto { | |||
| int id; | |||
| }; | |||
|
|
|||
There was a problem hiding this comment.
Please update the copyright year. https://github.com/tempesta-tech/tempesta/blob/master/.githooks/pre-commit#L12 should do this automatically, but it seems it didn't. @ai-tmpst you also had the same issue recently, did you cope it?
| unsigned short cipher_suite_hash; | ||
| unsigned short extension_type_hash; | ||
| unsigned short elliptic_curve_hash; | ||
| } __attribute__((packed)) TlsJa5t; |
There was a problem hiding this comment.
We have only one such structure per a TLS session, so I think it makes sense to keep the compiler alignment and not trying to save 1 byte. It seems we can just skip the first unnamed alignment bit field and let compiler to place data on it's own.
| unsigned short elliptic_curve_hash; | ||
| } __attribute__((packed)) TlsJa5t; | ||
|
|
||
|
|
| @@ -291,6 +319,7 @@ typedef struct { | |||
| unsigned char id_len; | |||
| unsigned char id[TTLS_SESS_ID_LEN]; | |||
| unsigned char master[48]; | |||
| TlsJa5t ja5t; | |||
There was a problem hiding this comment.
| TlsJa5t ja5t; | |
| TlsJa5t ja5t; |
Typically we align data types and member names in data structures
| @@ -720,6 +723,7 @@ ttls_parse_client_hello(TlsCtx *tls, unsigned char *buf, size_t len, | |||
| TlsIOCtx *io = &tls->io_in; | |||
| T_FSM_INIT(tls->state, "TLS ClientHello"); | |||
|
|
|||
|
|
|||
| if (unlikely(ext_type == TTLS_TLS_EXT_SESSION_TICKET)) | ||
| tmp = tls->hs->ticket_ctx.ticket; | ||
| memcpy_fast(tmp + io->rlen, p, n); | ||
| } | ||
| } |
There was a problem hiding this comment.
3 aleagnment breaks. Do you use tabs as 8 characters?
|
|
||
| /* ja5t must be initialized with zeros */ | ||
| tls->sess.ja5t.cipher_suite_hash *= TTLS_JA5_HASH_CALC_PRIME; | ||
| tls->sess.ja5t.cipher_suite_hash += cs;; |
There was a problem hiding this comment.
| tls->sess.ja5t.cipher_suite_hash += cs;; | |
| tls->sess.ja5t.cipher_suite_hash += cs; |
These 2 lines implement our hashing, which could change -what about to make a macro to compute a hash for any field? Like
#define TTLS_COMPUTE_JA5_ACCHASH(hash, field) \
do { \
(hash) *= TTLS_JA5_HASH_CALC_PRIME; \
(hash) += (field); \
} while (0) | if ((ret = ttls_parse_extension(tls, tmp, ext_sz, ext_type))) | ||
| return ret; | ||
| } | ||
|
|
There was a problem hiding this comment.
Senseless change with broken identation
tls/tls_srv.c
Outdated
| @@ -1121,6 +1142,10 @@ ttls_parse_client_hello(TlsCtx *tls, unsigned char *buf, size_t len, | |||
| */ | |||
| ttls_process_session_ticket(tls); | |||
|
|
|||
| /* JA5t computation */ | |||
| tls->sess.ja5t.is_abbreviated = tls->hs->resume; | |||
| tls->sess.ja5t.alpn = tls->alpn_chosen->id;; | |||
There was a problem hiding this comment.
Not exactly: this is choosen ALPN, but we need ALPN, sent by a client, so it seems ttls_parse_alpn_ext() is a better place. E.g. if we end up with HTTP/2, we still need to fingerprint http/1.1,h2. Ideally, if we can handle the difference between http/1.1,h2 and h2,http/1.1, but not necessary - if it causes overhead, we'll be just fine with setting bits for h2 or http/1.1
No description provided.