[
https://issues.apache.org/jira/browse/TS-4092?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15153615#comment-15153615
]
ASF GitHub Bot commented on TS-4092:
------------------------------------
Github user masaori335 commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/460#discussion_r53417506
--- Diff: proxy/http2/HTTP2.cc ---
@@ -484,246 +498,162 @@ convert_from_2_to_1_1_header(HTTPHdr *headers)
return PARSE_DONE;
}
-static int64_t
-http2_write_header_field(uint8_t *out, const uint8_t *end,
MIMEFieldWrapper &header, Http2IndexingTable &indexing_table)
-{
- HpackFieldType field_type = HPACK_FIELD_INDEXED_LITERAL;
-
- // Cookie less that 20 bytes and Authorization are never indexed
- // This policy is refer to Firefox and nghttp2
- int name_len = 0, value_len = 0;
- const char *name = header.name_get(&name_len);
- header.value_get(&value_len);
- if ((ptr_len_casecmp(name, name_len, MIME_FIELD_COOKIE, MIME_LEN_COOKIE)
== 0 && value_len < 20) ||
- (ptr_len_casecmp(name, name_len, MIME_FIELD_AUTHORIZATION,
MIME_LEN_AUTHORIZATION) == 0)) {
- field_type = HPACK_FIELD_NEVERINDEX_LITERAL;
- }
-
- // TODO Enable to configure selecting header field representation
+void
+http2_convert_header_from_1_1_to_2(HTTPHdr *headers)
+{
+ HTTPHdr tmp;
+ tmp.create(http_hdr_type_get(headers->m_http));
+ tmp.copy(headers);
+ headers->fields_clear();
+
+ if (http_hdr_type_get(tmp.m_http) == HTTP_TYPE_RESPONSE) {
+ char status_str[HTTP2_LEN_STATUS_VALUE_STR + 1];
+ snprintf(status_str, sizeof(status_str), "%d", tmp.status_get());
+
+ // Add ':status' header field
+ MIMEField *status_field = headers->field_create(HTTP2_VALUE_STATUS,
HTTP2_LEN_STATUS);
+ status_field->value_set(headers->m_heap, headers->m_mime, status_str,
HTTP2_LEN_STATUS_VALUE_STR);
+ headers->field_attach(status_field);
+
+ MIMEFieldIter field_iter;
+ for (MIMEField *field = tmp.iter_get_first(&field_iter); field !=
NULL; field = tmp.iter_get_next(&field_iter)) {
+ // Intermediaries SHOULD remove connection-specific header fields.
+ const char *name;
+ int name_len;
+ const char *value;
+ int value_len;
+ name = field->name_get(&name_len);
+ if ((name_len == MIME_LEN_CONNECTION && strncasecmp(name,
MIME_FIELD_CONNECTION, name_len) == 0) ||
+ (name_len == MIME_LEN_KEEP_ALIVE && strncasecmp(name,
MIME_FIELD_KEEP_ALIVE, name_len) == 0) ||
+ (name_len == MIME_LEN_PROXY_CONNECTION && strncasecmp(name,
MIME_FIELD_PROXY_CONNECTION, name_len) == 0) ||
+ (name_len == MIME_LEN_TRANSFER_ENCODING && strncasecmp(name,
MIME_FIELD_TRANSFER_ENCODING, name_len) == 0) ||
+ (name_len == MIME_LEN_UPGRADE && strncasecmp(name,
MIME_FIELD_UPGRADE, name_len) == 0)) {
+ continue;
+ }
- const Http2LookupIndexResult &result = indexing_table.get_index(header);
- if (result.index > 0) {
- if (result.value_is_indexed) {
- return encode_indexed_header_field(out, end, result.index);
- } else {
- return encode_literal_header_field_with_indexed_name(out, end,
header, result.index, indexing_table, field_type);
+ MIMEField *newfield;
+ name = field->name_get(&name_len);
+ newfield = headers->field_create(name, name_len);
+ value = field->value_get(&value_len);
+ newfield->value_set(headers->m_heap, headers->m_mime, value,
value_len);
+ tmp.field_delete(field);
+ headers->field_attach(newfield);
+ // Set sensitive flag (See RFC7541 7.1.3)
+ // - Authorization header obviously should not be indexed
+ // - Short Cookie header should not be indexed because of low entropy
+ if ((ptr_len_casecmp(name, name_len, MIME_FIELD_COOKIE,
MIME_LEN_COOKIE) == 0 && value_len < 20) ||
+ (ptr_len_casecmp(name, name_len, MIME_FIELD_AUTHORIZATION,
MIME_LEN_AUTHORIZATION) == 0)) {
+ newfield->m_sensitive = 1;
+ }
}
- } else {
- return encode_literal_header_field_with_new_name(out, end, header,
indexing_table, field_type);
}
+ tmp.destroy();
}
int64_t
-http2_write_psuedo_headers(HTTPHdr *in, uint8_t *out, uint64_t out_len,
Http2IndexingTable &indexing_table)
+http2_encode_header_blocks(HTTPHdr *in, uint8_t *out, uint32_t out_len,
HpackHandle &handle)
{
- uint8_t *p = out;
- uint8_t *end = out + out_len;
- int64_t len;
-
- ink_assert(http_hdr_type_get(in->m_http) != HTTP_TYPE_UNKNOWN);
-
- // TODO Check whether buffer size is enough
-
- // Set psuedo header
- if (http_hdr_type_get(in->m_http) == HTTP_TYPE_RESPONSE) {
- char status_str[HPACK_LEN_STATUS_VALUE_STR + 1];
- snprintf(status_str, sizeof(status_str), "%d", in->status_get());
-
- // Add 'Status:' dummy header field
- MIMEField *status_field = mime_field_create(in->m_heap,
in->m_http->m_fields_impl);
- mime_field_name_value_set(in->m_heap, in->m_mime, status_field, -1,
HPACK_VALUE_STATUS, HPACK_LEN_STATUS, status_str,
- HPACK_LEN_STATUS_VALUE_STR, 0,
HPACK_LEN_STATUS + HPACK_LEN_STATUS_VALUE_STR, true);
- mime_hdr_field_attach(in->m_mime, status_field, 1, NULL);
-
- // Encode psuedo headers by HPACK
- MIMEFieldWrapper header(status_field, in->m_heap,
in->m_http->m_fields_impl);
-
- len = http2_write_header_field(p, end, header, indexing_table);
- if (len == -1)
- return -1;
- p += len;
-
- // Remove dummy header field
- in->field_delete(HPACK_VALUE_STATUS, HPACK_LEN_STATUS);
- }
-
- return p - out;
-}
-
-int64_t
-http2_write_header_fragment(HTTPHdr *in, MIMEFieldIter &field_iter,
uint8_t *out, uint64_t out_len,
- Http2IndexingTable &indexing_table, bool &cont)
-{
- uint8_t *p = out;
- uint8_t *end = out + out_len;
- int64_t len;
-
- ink_assert(http_hdr_type_get(in->m_http) != HTTP_TYPE_UNKNOWN);
- ink_assert(in);
-
- // TODO Get a index value from the tables for the header field, and then
- // choose a representation type.
- // TODO Each indexing types per field should be passed by a caller,
HTTP/2
- // implementation.
-
- // Get first header field which is required encoding
- MIMEField *field;
- if (!field_iter.m_block) {
- field = in->iter_get_first(&field_iter);
- } else {
- field = in->iter_get(&field_iter);
- }
-
- // Set mime headers
- cont = false;
- for (; field != NULL; field = in->iter_get_next(&field_iter)) {
- // Intermediaries SHOULD remove connection-specific header fields.
- int name_len;
- const char *name = field->name_get(&name_len);
- if ((name_len == MIME_LEN_CONNECTION && strncasecmp(name,
MIME_FIELD_CONNECTION, name_len) == 0) ||
- (name_len == MIME_LEN_KEEP_ALIVE && strncasecmp(name,
MIME_FIELD_KEEP_ALIVE, name_len) == 0) ||
- (name_len == MIME_LEN_PROXY_CONNECTION && strncasecmp(name,
MIME_FIELD_PROXY_CONNECTION, name_len) == 0) ||
- (name_len == MIME_LEN_TRANSFER_ENCODING && strncasecmp(name,
MIME_FIELD_TRANSFER_ENCODING, name_len) == 0) ||
- (name_len == MIME_LEN_UPGRADE && strncasecmp(name,
MIME_FIELD_UPGRADE, name_len) == 0)) {
- continue;
- }
-
- MIMEFieldWrapper header(field, in->m_heap, in->m_http->m_fields_impl);
- if ((len = http2_write_header_field(p, end, header, indexing_table))
== -1) {
- if (p == out) {
- // no progress was made, header was too big for the buffer,
skipping for now
- continue;
- }
- if (!cont) {
- // Parsing a part of headers is done
- cont = true;
- return p - out;
- } else {
- // Parse error
- return -1;
- }
- }
- p += len;
- }
-
- // Parsing all headers is done
- return p - out;
+ // TODO: It would be better to split Cookie header value
+ return hpack_encode_header_block(handle, out, out_len, in);
}
/*
* Decode Header Blocks to Header List.
*/
int64_t
-http2_decode_header_blocks(HTTPHdr *hdr, const uint8_t *buf_start, const
uint8_t *buf_end, Http2IndexingTable &indexing_table,
+http2_decode_header_blocks(HTTPHdr *hdr, const uint8_t *buf_start, const
uint32_t buf_len, HpackHandle &handle,
bool &trailing_header)
{
- const uint8_t *cursor = buf_start;
- HdrHeap *heap = hdr->m_heap;
- HTTPHdrImpl *hh = hdr->m_http;
- bool header_field_started = false;
+ const MIMEField *field;
+ const char *value;
+ int len;
bool is_trailing_header = trailing_header;
+ int64_t result = hpack_decode_header_block(handle, hdr, buf_start,
buf_len);
- while (cursor < buf_end) {
- int64_t read_bytes = 0;
+ if (result < 0) {
+ return result;
+ }
- // decode a header field encoded by HPACK
- MIMEField *field = mime_field_create(heap, hh->m_fields_impl);
- MIMEFieldWrapper header(field, heap, hh->m_fields_impl);
- HpackFieldType ftype = hpack_parse_field_type(*cursor);
- switch (ftype) {
- case HPACK_FIELD_INDEX:
- read_bytes = decode_indexed_header_field(header, cursor, buf_end,
indexing_table);
- if (read_bytes == HPACK_ERROR_COMPRESSION_ERROR) {
- return HPACK_ERROR_COMPRESSION_ERROR;
- }
- cursor += read_bytes;
- header_field_started = true;
- break;
- case HPACK_FIELD_INDEXED_LITERAL:
- case HPACK_FIELD_NOINDEX_LITERAL:
- case HPACK_FIELD_NEVERINDEX_LITERAL:
- read_bytes = decode_literal_header_field(header, cursor, buf_end,
indexing_table);
- if (read_bytes == HPACK_ERROR_COMPRESSION_ERROR) {
- return HPACK_ERROR_COMPRESSION_ERROR;
- }
- cursor += read_bytes;
- header_field_started = true;
- break;
- case HPACK_FIELD_TABLESIZE_UPDATE:
- if (header_field_started) {
- return HPACK_ERROR_COMPRESSION_ERROR;
+ MIMEFieldIter iter;
+ unsigned int expected_pseudo_header_count = 4;
+ unsigned int pseudo_header_count = 0;
+
+ if (is_trailing_header) {
+ expected_pseudo_header_count = 0;
+ }
+ for (field = hdr->iter_get_first(&iter); field != NULL; field =
hdr->iter_get_next(&iter)) {
+ value = field->name_get(&len);
+ // Pseudo headers must appear before regular headers
+ if (len && value[0] == ':') {
+ ++pseudo_header_count;
+ if (pseudo_header_count > expected_pseudo_header_count) {
+ return -result;
--- End diff --
Is it necessary to return this? Those values are used in
`decode_header_blocks` in Http2Stream.h to detect COMPRESSION_ERROR or
PROTOCOL_ERROR.
How about returning Http2ErrorCode directly?
> Decouple HPACK from HTTP/2
> --------------------------
>
> Key: TS-4092
> URL: https://issues.apache.org/jira/browse/TS-4092
> Project: Traffic Server
> Issue Type: Improvement
> Components: HTTP/2
> Reporter: Masakazu Kitajo
> Assignee: Masakazu Kitajo
> Fix For: 6.2.0
>
>
> Our HTTP/2 implementation and HPACK implementation are coupled tightly. This
> is bad. It makes things complicated.
> I tried to write a test runner which uses [hpack-test-case
> |https://github.com/http2jp/hpack-test-case], however, I had to call
> functions in HTTP2.cc. Because HPACK.cc has only primitive encoder and
> decoder, and doesn't handle header blocks. HTTP2.cc covers not only RFC7540
> but also some part of RFC7541.
> On the other hand, HPACK.h exports pseudo header names as constants. They
> should be in HTTP2.h or MIME.h as WKS. We don't need them in HPACK
> implementation.
> Also, HPACK is used with QUIC (at least in current draft). We should decouple
> HPACK from HTTP/2 so that we can use the module with QUIC in the future.
> Once we have done this, we can write tests for these improvements more easily.
> TS-4002, TS-4061, TS-4014 and TS-3478
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
