You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by Randall Meyer <ra...@yahoo.com.INVALID> on 2021/10/05 17:02:31 UTC
[VOTE] new API for getting the SNI for a client connection
Hello!
I'd like to propose adding a new API get grab the SNI from the client connection.
const char * TSSslSNIGet(TSVConn sslp, int *length)
This would remove some of the redundant code in the rate_limit plugin but also would allow for the rate_limit plugin to be used under BoringSSL. The APIs between OpenSSL and BoringSSL here are pretty different here and don't have access to the same underlying structs. We already save off the name in the core (for both open and boring) and this API just exposes that value.
Here is the PR showing the changes (both the API addition and code cleanup). This would be split into 2 PRs if this API addition is accepted.
https://github.com/apache/trafficserver/pull/8313
-Randall
Re: [E] Re: [VOTE] new API for getting the SNI for a client connection
Posted by Walt Karas <wk...@yahooinc.com.INVALID>.
If we really want to swing for the bleachers, we could move all config into
a plugin. In a way that would allow a proxy to be managed with SNMP, gRPC,
etc. by replacing the plugin.
On Tue, Oct 5, 2021 at 12:33 PM Robert O Butts <ro...@apache.org> wrote:
> +1
>
> IMO we should have APIs for all SSL/SNI data.
>
> On Tue, Oct 5, 2021 at 11:25 AM Leif Hedstrom <zw...@apache.org> wrote:
>
> > +1.
> >
> > I think the same pattern is in a couple of other “example” plugins as
> > well, which could be cleaned up the same way.
> >
> > — Leif
> >
> >
> > > On Oct 5, 2021, at 11:02 AM, Randall Meyer <randallmeyer@yahoo.com
> .INVALID>
> > wrote:
> > >
> > > Hello!
> > > I'd like to propose adding a new API get grab the SNI from the client
> > connection.
> > >
> > > const char * TSSslSNIGet(TSVConn sslp, int *length)
> > >
> > > This would remove some of the redundant code in the rate_limit plugin
> > but also would allow for the rate_limit plugin to be used under
> BoringSSL.
> > The APIs between OpenSSL and BoringSSL here are pretty different here and
> > don't have access to the same underlying structs. We already save off the
> > name in the core (for both open and boring) and this API just exposes
> that
> > value.
> > >
> > > Here is the PR showing the changes (both the API addition and code
> > cleanup). This would be split into 2 PRs if this API addition is
> accepted.
> > >
> > >
> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_apache_trafficserver_pull_8313&d=DwIFaQ&c=sWW_bEwW_mLyN3Kx2v57Q8e-CRbmiT9yOhqES_g_wVY&r=bRwMsdu0uyXK0N-ngQ91KF2IzpTwU-AXjOlCZnyKc_4&m=Tw5Nx95QZ9igI8Hl65kqtpzUO_yfLgq3zxd0vZPNIdw&s=wRrrS5YvSxROfyJVZrMRdrD_iC_dcAqotGW6eTvh5eM&e=
> > >
> > > -Randall
> >
> >
>
Re: [VOTE] new API for getting the SNI for a client connection
Posted by Sudheer Vinukonda <su...@yahoo.com.INVALID>.
+1
Sent from my iPhone
> On Oct 5, 2021, at 6:29 PM, Brian Neradt <br...@gmail.com> wrote:
>
> +1
>
> Thanks Randall.
>
>> On Tue, Oct 5, 2021 at 12:33 PM Robert O Butts <ro...@apache.org> wrote:
>>
>> +1
>>
>> IMO we should have APIs for all SSL/SNI data.
>>
>>> On Tue, Oct 5, 2021 at 11:25 AM Leif Hedstrom <zw...@apache.org> wrote:
>>>
>>> +1.
>>>
>>> I think the same pattern is in a couple of other “example” plugins as
>>> well, which could be cleaned up the same way.
>>>
>>> — Leif
>>>
>>>
>>>> On Oct 5, 2021, at 11:02 AM, Randall Meyer <randallmeyer@yahoo.com
>> .INVALID>
>>> wrote:
>>>>
>>>> Hello!
>>>> I'd like to propose adding a new API get grab the SNI from the client
>>> connection.
>>>>
>>>> const char * TSSslSNIGet(TSVConn sslp, int *length)
>>>>
>>>> This would remove some of the redundant code in the rate_limit plugin
>>> but also would allow for the rate_limit plugin to be used under
>> BoringSSL.
>>> The APIs between OpenSSL and BoringSSL here are pretty different here and
>>> don't have access to the same underlying structs. We already save off the
>>> name in the core (for both open and boring) and this API just exposes
>> that
>>> value.
>>>>
>>>> Here is the PR showing the changes (both the API addition and code
>>> cleanup). This would be split into 2 PRs if this API addition is
>> accepted.
>>>>
>>>> https://github.com/apache/trafficserver/pull/8313
>>>>
>>>> -Randall
>>>
>>>
>>
> --
> "Come to Me, all who are weary and heavy-laden, and I will
> give you rest. Take My yoke upon you and learn from Me, for
> I am gentle and humble in heart, and you will find rest for
> your souls. For My yoke is easy and My burden is light."
>
> ~ Matthew 11:28-30
Re: [VOTE] new API for getting the SNI for a client connection
Posted by Brian Neradt <br...@gmail.com>.
+1
Thanks Randall.
On Tue, Oct 5, 2021 at 12:33 PM Robert O Butts <ro...@apache.org> wrote:
> +1
>
> IMO we should have APIs for all SSL/SNI data.
>
> On Tue, Oct 5, 2021 at 11:25 AM Leif Hedstrom <zw...@apache.org> wrote:
>
> > +1.
> >
> > I think the same pattern is in a couple of other “example” plugins as
> > well, which could be cleaned up the same way.
> >
> > — Leif
> >
> >
> > > On Oct 5, 2021, at 11:02 AM, Randall Meyer <randallmeyer@yahoo.com
> .INVALID>
> > wrote:
> > >
> > > Hello!
> > > I'd like to propose adding a new API get grab the SNI from the client
> > connection.
> > >
> > > const char * TSSslSNIGet(TSVConn sslp, int *length)
> > >
> > > This would remove some of the redundant code in the rate_limit plugin
> > but also would allow for the rate_limit plugin to be used under
> BoringSSL.
> > The APIs between OpenSSL and BoringSSL here are pretty different here and
> > don't have access to the same underlying structs. We already save off the
> > name in the core (for both open and boring) and this API just exposes
> that
> > value.
> > >
> > > Here is the PR showing the changes (both the API addition and code
> > cleanup). This would be split into 2 PRs if this API addition is
> accepted.
> > >
> > > https://github.com/apache/trafficserver/pull/8313
> > >
> > > -Randall
> >
> >
>
--
"Come to Me, all who are weary and heavy-laden, and I will
give you rest. Take My yoke upon you and learn from Me, for
I am gentle and humble in heart, and you will find rest for
your souls. For My yoke is easy and My burden is light."
~ Matthew 11:28-30
Re: [VOTE] new API for getting the SNI for a client connection
Posted by Robert O Butts <ro...@apache.org>.
+1
IMO we should have APIs for all SSL/SNI data.
On Tue, Oct 5, 2021 at 11:25 AM Leif Hedstrom <zw...@apache.org> wrote:
> +1.
>
> I think the same pattern is in a couple of other “example” plugins as
> well, which could be cleaned up the same way.
>
> — Leif
>
>
> > On Oct 5, 2021, at 11:02 AM, Randall Meyer <ra...@yahoo.com.INVALID>
> wrote:
> >
> > Hello!
> > I'd like to propose adding a new API get grab the SNI from the client
> connection.
> >
> > const char * TSSslSNIGet(TSVConn sslp, int *length)
> >
> > This would remove some of the redundant code in the rate_limit plugin
> but also would allow for the rate_limit plugin to be used under BoringSSL.
> The APIs between OpenSSL and BoringSSL here are pretty different here and
> don't have access to the same underlying structs. We already save off the
> name in the core (for both open and boring) and this API just exposes that
> value.
> >
> > Here is the PR showing the changes (both the API addition and code
> cleanup). This would be split into 2 PRs if this API addition is accepted.
> >
> > https://github.com/apache/trafficserver/pull/8313
> >
> > -Randall
>
>
Re: [VOTE] new API for getting the SNI for a client connection
Posted by Leif Hedstrom <zw...@apache.org>.
+1.
I think the same pattern is in a couple of other “example” plugins as well, which could be cleaned up the same way.
— Leif
> On Oct 5, 2021, at 11:02 AM, Randall Meyer <ra...@yahoo.com.INVALID> wrote:
>
> Hello!
> I'd like to propose adding a new API get grab the SNI from the client connection.
>
> const char * TSSslSNIGet(TSVConn sslp, int *length)
>
> This would remove some of the redundant code in the rate_limit plugin but also would allow for the rate_limit plugin to be used under BoringSSL. The APIs between OpenSSL and BoringSSL here are pretty different here and don't have access to the same underlying structs. We already save off the name in the core (for both open and boring) and this API just exposes that value.
>
> Here is the PR showing the changes (both the API addition and code cleanup). This would be split into 2 PRs if this API addition is accepted.
>
> https://github.com/apache/trafficserver/pull/8313
>
> -Randall
Re: [VOTE] new API for getting the SNI for a client connection
Posted by Randall Meyer <ra...@yahoo.com.INVALID>.
I'm calling this vote with 5 +1 votes, and no -1’s. I'm going to name the API TSVConnSslSniGet like Masakazu suggested.
The PR with this change is here:
https://github.com/apache/trafficserver/pull/8313
Thank you for all the comments received!
-Randall
On Thursday, October 7, 2021, 07:53:38 AM PDT, Leif Hedstrom <zw...@apache.org> wrote:
> On Oct 7, 2021, at 1:54 AM, Masakazu Kitajo <ma...@apache.org> wrote:
>
> I'm +1 on adding it, but not sure about the name.
>
> There are a couple of similar APIs such as TSVConnSslCipherGet and
> TSVConnSslProtocolGet, and those start with "TSVConnSsl". TSVConnSslSNIGet
> may be more consistent with existing API names. It seems like TSSsl
> (without VConn) is used for APIs that purely interact with SSL stuff. Also
> only the first characters of acronyms are capital in many cases (not all).
> In that sense TSVConnSslSniGet, maybe.
Good point, and seeing that this acts on a TSVConn, we ought to stick to that naming convention.
— Leif
>
> Masakazu
>
> On Wed, Oct 6, 2021 at 2:02 AM Randall Meyer <ra...@yahoo.com.invalid>
> wrote:
>
>> Hello!
>> I'd like to propose adding a new API get grab the SNI from the client
>> connection.
>>
>> const char * TSSslSNIGet(TSVConn sslp, int *length)
>>
>> This would remove some of the redundant code in the rate_limit plugin but
>> also would allow for the rate_limit plugin to be used under BoringSSL. The
>> APIs between OpenSSL and BoringSSL here are pretty different here and don't
>> have access to the same underlying structs. We already save off the name in
>> the core (for both open and boring) and this API just exposes that value.
>>
>> Here is the PR showing the changes (both the API addition and code
>> cleanup). This would be split into 2 PRs if this API addition is accepted.
>>
>> https://github.com/apache/trafficserver/pull/8313
>>
>> -Randall
>>
Re: [VOTE] new API for getting the SNI for a client connection
Posted by Leif Hedstrom <zw...@apache.org>.
> On Oct 7, 2021, at 1:54 AM, Masakazu Kitajo <ma...@apache.org> wrote:
>
> I'm +1 on adding it, but not sure about the name.
>
> There are a couple of similar APIs such as TSVConnSslCipherGet and
> TSVConnSslProtocolGet, and those start with "TSVConnSsl". TSVConnSslSNIGet
> may be more consistent with existing API names. It seems like TSSsl
> (without VConn) is used for APIs that purely interact with SSL stuff. Also
> only the first characters of acronyms are capital in many cases (not all).
> In that sense TSVConnSslSniGet, maybe.
Good point, and seeing that this acts on a TSVConn, we ought to stick to that naming convention.
— Leif
>
> Masakazu
>
> On Wed, Oct 6, 2021 at 2:02 AM Randall Meyer <ra...@yahoo.com.invalid>
> wrote:
>
>> Hello!
>> I'd like to propose adding a new API get grab the SNI from the client
>> connection.
>>
>> const char * TSSslSNIGet(TSVConn sslp, int *length)
>>
>> This would remove some of the redundant code in the rate_limit plugin but
>> also would allow for the rate_limit plugin to be used under BoringSSL. The
>> APIs between OpenSSL and BoringSSL here are pretty different here and don't
>> have access to the same underlying structs. We already save off the name in
>> the core (for both open and boring) and this API just exposes that value.
>>
>> Here is the PR showing the changes (both the API addition and code
>> cleanup). This would be split into 2 PRs if this API addition is accepted.
>>
>> https://github.com/apache/trafficserver/pull/8313
>>
>> -Randall
>>
Re: [VOTE] new API for getting the SNI for a client connection
Posted by Masakazu Kitajo <ma...@apache.org>.
I'm +1 on adding it, but not sure about the name.
There are a couple of similar APIs such as TSVConnSslCipherGet and
TSVConnSslProtocolGet, and those start with "TSVConnSsl". TSVConnSslSNIGet
may be more consistent with existing API names. It seems like TSSsl
(without VConn) is used for APIs that purely interact with SSL stuff. Also
only the first characters of acronyms are capital in many cases (not all).
In that sense TSVConnSslSniGet, maybe.
Masakazu
On Wed, Oct 6, 2021 at 2:02 AM Randall Meyer <ra...@yahoo.com.invalid>
wrote:
> Hello!
> I'd like to propose adding a new API get grab the SNI from the client
> connection.
>
> const char * TSSslSNIGet(TSVConn sslp, int *length)
>
> This would remove some of the redundant code in the rate_limit plugin but
> also would allow for the rate_limit plugin to be used under BoringSSL. The
> APIs between OpenSSL and BoringSSL here are pretty different here and don't
> have access to the same underlying structs. We already save off the name in
> the core (for both open and boring) and this API just exposes that value.
>
> Here is the PR showing the changes (both the API addition and code
> cleanup). This would be split into 2 PRs if this API addition is accepted.
>
> https://github.com/apache/trafficserver/pull/8313
>
> -Randall
>