bneradt commented on code in PR #11424:
URL: https://github.com/apache/trafficserver/pull/11424#discussion_r1631435243
##########
plugins/ja3_fingerprint/ja3_fingerprint.cc:
##########
@@ -126,52 +94,43 @@ getIP(sockaddr const *s_sockaddr, char
res[INET6_ADDRSTRLEN])
}
static std::string
-custom_get_ja3(SSL *s)
+custom_get_ja3(SSL *ssl)
Review Comment:
Just checking: can this not be const? I'm assuming the APIs probably take
them non-const which prevents that, but it seems a shame since the plugin
should function read-only on this stuff.
##########
plugins/ja3_fingerprint/ja3_fingerprint.cc:
##########
@@ -126,52 +94,43 @@ getIP(sockaddr const *s_sockaddr, char
res[INET6_ADDRSTRLEN])
}
static std::string
-custom_get_ja3(SSL *s)
+custom_get_ja3(SSL *ssl)
{
- std::string ja3;
- size_t len;
- const unsigned char *p;
+ std::string result;
+ std::size_t len;
+ const unsigned char *buf;
Review Comment:
While you're at it, let's initialize the variables (I know it wasn't before
your patch).
##########
plugins/ja3_fingerprint/ja3_utils.h:
##########
@@ -0,0 +1,65 @@
+/** @ja3_utils.h
+ Plugin JA3 Fingerprint calculates JA3 signatures for incoming SSL traffic.
+ @section license License
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+ http://www.apache.org/licenses/LICENSE-2.0
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include <string>
+
+namespace ja3
+{
+
+/** Encode a buffer of 8bit values.
+ *
+ * The values will be converted to their decimal string representations and
+ * joined with the '-' character.
+ *
+ * @param buf The buffer to encode. This should be an SSL buffer of 8bit
+ * values.
+ * @param len The length of the buffer. If the length is zero, buf will
+ * not be dereferenced.
+ * @return The string-encoded ja3 representation of the buffer.
+ */
+std::string encode_word_buffer(unsigned char const *buf, int const len);
+
+/** Encode a buffer of big-endian 16bit values.
+ *
+ * The values will be converted to their decimal string representations and
+ * joined with the '-' character. Any GREASE values in the buffer will be
+ * ignored.
+ *
+ * @param buf The buffer to encode. This should be a big-endian SSL buffer
+ * of 16bit values.
+ * @param len The length of the buffer. If the length is zero, buf will not
+ * be dereferenced.
+ * @return The string-encoded ja3 representation of the buffer.
+ */
+std::string encode_dword_buffer(unsigned char const *buf, int const len);
+
+/** Encode a buffer of integers.
+ *
+ * The values will be converted to their decimal string representations and
+ * joined with the '-' character. Any GREASE values in the buffer will be
+ * ignored.
+ *
+ * @param buf The buffer to encode. The buffer underlying the span should be
+ * an SSL buffer of ints.
+ * @param len The length (number of values) in the buffer. If the length is
+ * zero, buf will not be dereferenced.
+ * @return The string-encoded ja3 representation of the buffer.
+ */
+std::string encode_integer_buffer(int const *buf, int const len);
Review Comment:
Thanks for right side const. :)
Copy by value parameters aren't typically made const though (`int len`).
##########
plugins/ja3_fingerprint/ja3_utils.cc:
##########
@@ -0,0 +1,102 @@
+/** @ja3_utils.cc
Review Comment:
I know this is copied from the other ja3_fingerprint.cc header file, but I
think it's supposed to be `@file ja3_utils.cc`. Can you fix it in the
ja3_fingerprint.cc file too?
##########
plugins/ja3_fingerprint/ja3_utils.cc:
##########
@@ -0,0 +1,102 @@
+/** @ja3_utils.cc
+ Plugin JA3 Fingerprint calculates JA3 signatures for incoming SSL traffic.
+ @section license License
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+ http://www.apache.org/licenses/LICENSE-2.0
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include <algorithm>
+#include <cstdint>
+#include <string>
+#include <unordered_set>
+
+namespace ja3
+{
+
+// GREASE table as in ja3
+static std::unordered_set<std::uint16_t> const GREASE_table = {0x0a0a, 0x1a1a,
0x2a2a, 0x3a3a, 0x4a4a, 0x5a5a, 0x6a6a, 0x7a7a,
+ 0x8a8a, 0x9a9a,
0xaaaa, 0xbaba, 0xcaca, 0xdada, 0xeaea, 0xfafa};
+
+static constexpr std::uint16_t
+from_big_endian(unsigned char lowbyte, unsigned char highbyte)
+{
+ return (static_cast<std::uint16_t>(lowbyte) << 8) + highbyte;
+}
+
+static bool
+ja3_should_ignore(std::uint16_t n)
+{
+ return GREASE_table.find(n) != GREASE_table.end();
+}
+
+std::string
+encode_word_buffer(unsigned char const *buf, int const len)
+{
+ std::string result;
+ if (len > 0) {
+ // Benchmarks show that reserving space in the string here would cause
+ // a 40% increase in runtime for a buffer with 10 elements... so we
+ // don't do it.
+ result.append(std::to_string(buf[0]));
+ std::for_each(buf + 1, buf + len, [&result](unsigned char i) {
+ result.push_back('-');
+ result.append(std::to_string(i));
+ });
+ }
+ return result;
+}
+
+std::string
+encode_dword_buffer(unsigned char const *buf, int const len)
+{
+ std::string result;
+ auto it{buf};
+ while (it < buf + len && ja3_should_ignore(from_big_endian(it[0], it[1]))) {
Review Comment:
I suggest adding some parens to help the reader a bit:
```cpp
while ((it < (buf + len)) && ja3_should_ignore(from_big_endian(it[0],
it[1]))) {
```
##########
plugins/ja3_fingerprint/ja3_utils.cc:
##########
@@ -0,0 +1,102 @@
+/** @ja3_utils.cc
+ Plugin JA3 Fingerprint calculates JA3 signatures for incoming SSL traffic.
+ @section license License
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+ http://www.apache.org/licenses/LICENSE-2.0
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include <algorithm>
+#include <cstdint>
+#include <string>
+#include <unordered_set>
+
+namespace ja3
+{
+
+// GREASE table as in ja3
+static std::unordered_set<std::uint16_t> const GREASE_table = {0x0a0a, 0x1a1a,
0x2a2a, 0x3a3a, 0x4a4a, 0x5a5a, 0x6a6a, 0x7a7a,
+ 0x8a8a, 0x9a9a,
0xaaaa, 0xbaba, 0xcaca, 0xdada, 0xeaea, 0xfafa};
+
+static constexpr std::uint16_t
+from_big_endian(unsigned char lowbyte, unsigned char highbyte)
+{
+ return (static_cast<std::uint16_t>(lowbyte) << 8) + highbyte;
Review Comment:
Bitwise or (`|`) maybe? Could be slightly more efficient.
##########
plugins/ja3_fingerprint/ja3_utils.cc:
##########
@@ -0,0 +1,102 @@
+/** @ja3_utils.cc
+ Plugin JA3 Fingerprint calculates JA3 signatures for incoming SSL traffic.
+ @section license License
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+ http://www.apache.org/licenses/LICENSE-2.0
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include <algorithm>
+#include <cstdint>
+#include <string>
+#include <unordered_set>
+
+namespace ja3
+{
+
+// GREASE table as in ja3
+static std::unordered_set<std::uint16_t> const GREASE_table = {0x0a0a, 0x1a1a,
0x2a2a, 0x3a3a, 0x4a4a, 0x5a5a, 0x6a6a, 0x7a7a,
+ 0x8a8a, 0x9a9a,
0xaaaa, 0xbaba, 0xcaca, 0xdada, 0xeaea, 0xfafa};
+
+static constexpr std::uint16_t
+from_big_endian(unsigned char lowbyte, unsigned char highbyte)
+{
+ return (static_cast<std::uint16_t>(lowbyte) << 8) + highbyte;
+}
+
+static bool
+ja3_should_ignore(std::uint16_t n)
+{
+ return GREASE_table.find(n) != GREASE_table.end();
+}
+
+std::string
+encode_word_buffer(unsigned char const *buf, int const len)
+{
+ std::string result;
+ if (len > 0) {
+ // Benchmarks show that reserving space in the string here would cause
+ // a 40% increase in runtime for a buffer with 10 elements... so we
+ // don't do it.
+ result.append(std::to_string(buf[0]));
+ std::for_each(buf + 1, buf + len, [&result](unsigned char i) {
+ result.push_back('-');
+ result.append(std::to_string(i));
+ });
+ }
+ return result;
+}
+
+std::string
+encode_dword_buffer(unsigned char const *buf, int const len)
+{
+ std::string result;
+ auto it{buf};
+ while (it < buf + len && ja3_should_ignore(from_big_endian(it[0], it[1]))) {
+ it += 2;
+ }
+ if (it < buf + len) {
+ // Benchmarks show that reserving buf.size() - 1 space in the string here
+ // would have no impact on performance. Since the string may not even need
+ // that much due to GREASE values present in the buffer, we don't do it.
+ result.append(std::to_string(from_big_endian(it[0], it[1])));
+ it += 2;
+ for (; it < buf + len; it += 2) {
+ auto value{from_big_endian(it[0], it[1])};
Review Comment:
might as well `const`
##########
plugins/ja3_fingerprint/ja3_utils.cc:
##########
@@ -0,0 +1,104 @@
+/** @ja3_utils.cc
+ Plugin JA3 Fingerprint calculates JA3 signatures for incoming SSL traffic.
+ @section license License
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+ http://www.apache.org/licenses/LICENSE-2.0
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include <algorithm>
+#include <cstdint>
+#include <string>
+#include <unordered_set>
+
+namespace ja3
+{
+
+// GREASE table as in ja3
+static std::unordered_set<std::uint16_t> const GREASE_table = {0x0a0a, 0x1a1a,
0x2a2a, 0x3a3a, 0x4a4a, 0x5a5a, 0x6a6a, 0x7a7a,
+ 0x8a8a, 0x9a9a,
0xaaaa, 0xbaba, 0xcaca, 0xdada, 0xeaea, 0xfafa};
+
+static constexpr std::uint16_t
+from_big_endian(unsigned char lowbyte, unsigned char highbyte)
+{
+ return (static_cast<std::uint16_t>(lowbyte) << 8) + highbyte;
+}
+
+static bool
+ja3_should_ignore(std::uint16_t n)
+{
+ return GREASE_table.find(n) != GREASE_table.end();
+}
+
+std::string
+encode_word_buffer(unsigned char const *buf, int const len)
+{
+ std::string result;
+ if (len > 0) {
+ // Benchmarks show that reserving space in the string here would cause
+ // a 40% increase in runtime for a buffer with 10 elements... so we
+ // don't do it.
Review Comment:
That's interesting.
##########
plugins/ja3_fingerprint/ja3_utils.cc:
##########
@@ -0,0 +1,102 @@
+/** @ja3_utils.cc
+ Plugin JA3 Fingerprint calculates JA3 signatures for incoming SSL traffic.
+ @section license License
Review Comment:
Probably good to space this out a bit, like here:
https://github.com/apache/trafficserver/blob/master/src/proxy/http/HttpSM.cc#L3
##########
plugins/ja3_fingerprint/ja3_utils.cc:
##########
@@ -0,0 +1,102 @@
+/** @ja3_utils.cc
+ Plugin JA3 Fingerprint calculates JA3 signatures for incoming SSL traffic.
+ @section license License
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+ http://www.apache.org/licenses/LICENSE-2.0
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include <algorithm>
+#include <cstdint>
+#include <string>
+#include <unordered_set>
+
+namespace ja3
+{
+
+// GREASE table as in ja3
+static std::unordered_set<std::uint16_t> const GREASE_table = {0x0a0a, 0x1a1a,
0x2a2a, 0x3a3a, 0x4a4a, 0x5a5a, 0x6a6a, 0x7a7a,
+ 0x8a8a, 0x9a9a,
0xaaaa, 0xbaba, 0xcaca, 0xdada, 0xeaea, 0xfafa};
+
+static constexpr std::uint16_t
+from_big_endian(unsigned char lowbyte, unsigned char highbyte)
+{
+ return (static_cast<std::uint16_t>(lowbyte) << 8) + highbyte;
+}
+
+static bool
+ja3_should_ignore(std::uint16_t n)
+{
+ return GREASE_table.find(n) != GREASE_table.end();
+}
+
+std::string
+encode_word_buffer(unsigned char const *buf, int const len)
+{
+ std::string result;
+ if (len > 0) {
+ // Benchmarks show that reserving space in the string here would cause
+ // a 40% increase in runtime for a buffer with 10 elements... so we
+ // don't do it.
+ result.append(std::to_string(buf[0]));
+ std::for_each(buf + 1, buf + len, [&result](unsigned char i) {
+ result.push_back('-');
+ result.append(std::to_string(i));
+ });
Review Comment:
Nice use of algorithms.
##########
plugins/ja3_fingerprint/ja3_utils.cc:
##########
@@ -0,0 +1,102 @@
+/** @ja3_utils.cc
+ Plugin JA3 Fingerprint calculates JA3 signatures for incoming SSL traffic.
+ @section license License
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+ http://www.apache.org/licenses/LICENSE-2.0
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include <algorithm>
+#include <cstdint>
+#include <string>
+#include <unordered_set>
+
+namespace ja3
+{
+
+// GREASE table as in ja3
+static std::unordered_set<std::uint16_t> const GREASE_table = {0x0a0a, 0x1a1a,
0x2a2a, 0x3a3a, 0x4a4a, 0x5a5a, 0x6a6a, 0x7a7a,
+ 0x8a8a, 0x9a9a,
0xaaaa, 0xbaba, 0xcaca, 0xdada, 0xeaea, 0xfafa};
+
+static constexpr std::uint16_t
+from_big_endian(unsigned char lowbyte, unsigned char highbyte)
+{
+ return (static_cast<std::uint16_t>(lowbyte) << 8) + highbyte;
+}
+
+static bool
+ja3_should_ignore(std::uint16_t n)
+{
+ return GREASE_table.find(n) != GREASE_table.end();
+}
+
+std::string
+encode_word_buffer(unsigned char const *buf, int const len)
+{
+ std::string result;
+ if (len > 0) {
+ // Benchmarks show that reserving space in the string here would cause
+ // a 40% increase in runtime for a buffer with 10 elements... so we
+ // don't do it.
+ result.append(std::to_string(buf[0]));
+ std::for_each(buf + 1, buf + len, [&result](unsigned char i) {
+ result.push_back('-');
+ result.append(std::to_string(i));
+ });
+ }
+ return result;
+}
+
+std::string
+encode_dword_buffer(unsigned char const *buf, int const len)
+{
+ std::string result;
+ auto it{buf};
+ while (it < buf + len && ja3_should_ignore(from_big_endian(it[0], it[1]))) {
+ it += 2;
+ }
+ if (it < buf + len) {
Review Comment:
```cpp
if (it < (buf + len)) {
##########
plugins/ja3_fingerprint/ja3_utils.h:
##########
@@ -0,0 +1,65 @@
+/** @ja3_utils.h
+ Plugin JA3 Fingerprint calculates JA3 signatures for incoming SSL traffic.
+ @section license License
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+ http://www.apache.org/licenses/LICENSE-2.0
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include <string>
+
+namespace ja3
+{
+
+/** Encode a buffer of 8bit values.
+ *
+ * The values will be converted to their decimal string representations and
+ * joined with the '-' character.
+ *
+ * @param buf The buffer to encode. This should be an SSL buffer of 8bit
+ * values.
+ * @param len The length of the buffer. If the length is zero, buf will
+ * not be dereferenced.
+ * @return The string-encoded ja3 representation of the buffer.
+ */
+std::string encode_word_buffer(unsigned char const *buf, int const len);
+
+/** Encode a buffer of big-endian 16bit values.
+ *
+ * The values will be converted to their decimal string representations and
+ * joined with the '-' character. Any GREASE values in the buffer will be
+ * ignored.
+ *
+ * @param buf The buffer to encode. This should be a big-endian SSL buffer
+ * of 16bit values.
+ * @param len The length of the buffer. If the length is zero, buf will not
+ * be dereferenced.
+ * @return The string-encoded ja3 representation of the buffer.
+ */
+std::string encode_dword_buffer(unsigned char const *buf, int const len);
Review Comment:
I know you're not introducing this terminology here, but isn't this the
typical names of things?
* `byte`: 8 bits
* `word`: 16 bits
* `dword`: 32 bits
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
