Bug 137425

Comment 1 Edwin Groothuis 2009-08-04 15:00:21 UTC

Maintainer of net/freeradius2,

Please note that PR ports/137425 has just been submitted.

If it contains a patch for an upgrade, an enhancement or a bug fix
you agree on, reply to this email stating that you approve the patch
and a committer will take care of it.

The full text of the PR can be found at:
    http://www.freebsd.org/cgi/query-pr.cgi?pr=ports/137425

-- 
Edwin Groothuis via the GNATS Auto Assign Tool
edwin@FreeBSD.org

Comment 2 Edwin Groothuis 2009-08-04 15:00:23 UTC

State Changed
From-To: open->feedback

Awaiting maintainers feedback (via the GNATS Auto Assign Tool)

Comment 3 Wesley Shields 2009-08-04 15:02:26 UTC

Responsible Changed
From-To: freebsd-ports-bugs->wxs

I'll take it.

Thanks very for this, Ryan - others have mentioned Oracle support, but 
nobody has come up with a patch before, let alone a complete patch in a 
PR.


There's a few things I'd like to sort out before this is committed.


I think ORACLE is a better name for the option and plist variable than 
the rather cryptic OCI8.


The (EXPERIMENTAL) label in an option means that the EXPERIMENTAL switch 
is implied by that option. I believe you are declaring Oracle support in 
FreeRADIUS on FreeBSD to be experimental, but that you don't need 
EXPERIMENTAL set. If so, I think we need to explain this a different 
way, rather than overloading the (EXPERIMENTAL) label.


Can you explain the patch to src/main/exec.c? It would be good to know 
where it has come from (for example, is it mentioned in the 
documentation for rlm_sql_oracle, or on bugs.freeradius.org?) and why it 
is needed. I see what it does, but with FreeRADIUS being a security 
critical port, I'd rather understand it fully before this is committed.

I've tried to limit patches in the FreeRADIUS ports to the build system 
only since I took over maintainership, but sometimes patches to the 
actual code are necessary, and this seems to be one of those occasions. 
The Debian OpenSSL debacle stands out in my mind as a demonstration of 
the danger of single line changes to security critical code, though I 
realise this patch doesn't touch cryptographic code.


PORTREVISION should arguably be bumped.


The patch as it stands is not approved, but I'm sure we can sort out the 
minor changes to get it into shape. I'm very grateful for your 
submission and look forward to devising an approved patch together.


Unfortunately I'm away for most of the next two and a half weeks, but 
I'll try to keep an eye on this PR via my laptop, especially with the 
FreeBSD 8.0 ports freeze so close.


With best wishes,




David (maintainer, net/freeradius2)
-- 
David Wood
david@wood2.org.uk

David,

I selected OCI8 as it appears to be what is used in other ports, specifically referencing the oracle 8 client that lives in ports.   There is a version 7 of the client (databases/oracle7-client) so I specifically wanted to ensure we ended up with version 8 of the client.  www/codeigniter and databases/p5-DBD-Oracle are examples of where WITH_OCI8 is used elsewhere within the ports tree.

You are correct about the EXPERIMENTAL label meaning that oracle support under freebsd is experimental, not that we need the EXPERIMENTAL module set to use oracle.

As for the patch to src/main/exec.c, the Oracle libraries appear to hijack various signals for some silly reason.  The hijacking of the SIGCHLD signal breaks FR's ability to use exec as intended, which is required by the mschap module as well as the exec module.  Without the patch, FR doesn't think that the process it exec'd completed successfully and, as you can imagine, this creates other issues.

The Oracle documentation that I have come across in the past says "The pipe driver uses SIGCLD, also referred to as SIGCHLD, when an Oracle process dies."  Since FR actually forks() to use exec, I believe this is where the Oracle libraries cause havoc for us.

The patch simply resets the sighandler back to the default before forking, which (in my testing) has resolved the issue.  Since it doesn't appear as if FR defines a special handler for SIGCHLD, I feel the default is appropriate.  There may be a more appropriate way of dealing with this, but, this seems to work.  Considering how minimal of a patch it is, I have decided to submit it as is.  However, I'd love to hear/see an alternative.

I have been running FR2 with the patch to exec.c (and with oracle support) for a few months now in a production environment on three separate hosts with great success.  I have, however, seen FR segfault on exit (with the Oracle libraries loaded, which I believe happens whenever detach_modules() is called) and therefor have flagged Oracle support as being experimental.

As far as PORTREVISION goes, I agree and apologize for missing that in my original patch.

I will submit a new patch that addresses the EXPERIMENTAL label and PORTREVISION.

Thanks,
-r


On (08/06/09 22:57), David Wood wrote:
> Thanks very for this, Ryan - others have mentioned Oracle support, but 
> nobody has come up with a patch before, let alone a complete patch in a 
> PR.
> 
> 
> There's a few things I'd like to sort out before this is committed.
> 
> 
> I think ORACLE is a better name for the option and plist variable than 
> the rather cryptic OCI8.
> 
> 
> The (EXPERIMENTAL) label in an option means that the EXPERIMENTAL switch 
> is implied by that option. I believe you are declaring Oracle support in 
> FreeRADIUS on FreeBSD to be experimental, but that you don't need 
> EXPERIMENTAL set. If so, I think we need to explain this a different 
> way, rather than overloading the (EXPERIMENTAL) label.
> 
> 
> Can you explain the patch to src/main/exec.c? It would be good to know 
> where it has come from (for example, is it mentioned in the 
> documentation for rlm_sql_oracle, or on bugs.freeradius.org?) and why it 
> is needed. I see what it does, but with FreeRADIUS being a security 
> critical port, I'd rather understand it fully before this is committed.
> 
> I've tried to limit patches in the FreeRADIUS ports to the build system 
> only since I took over maintainership, but sometimes patches to the 
> actual code are necessary, and this seems to be one of those occasions. 
> The Debian OpenSSL debacle stands out in my mind as a demonstration of 
> the danger of single line changes to security critical code, though I 
> realise this patch doesn't touch cryptographic code.
> 
> 
> PORTREVISION should arguably be bumped.
> 
> 
> The patch as it stands is not approved, but I'm sure we can sort out the 
> minor changes to get it into shape. I'm very grateful for your 
> submission and look forward to devising an approved patch together.
> 
> 
> Unfortunately I'm away for most of the next two and a half weeks, but 
> I'll try to keep an eye on this PR via my laptop, especially with the 
> FreeBSD 8.0 ports freeze so close.
> 
> 
> With best wishes,
> 
> 
> 
> 
> David (maintainer, net/freeradius2)
> -- 
> David Wood
> david@wood2.org.uk

-- 
Ryan Steinmetz
Lead Security/Systems Administrator
Infrastructure Engineering
Rochester Institute of Technology
585.475.5663
PGP: EF36 D45A 5CA9 28B1 A550  18CD A43C D111 7AD7 FAF2

Updated patch attached.

On (08/06/09 22:57), David Wood wrote:
> Thanks very for this, Ryan - others have mentioned Oracle support, but 
> nobody has come up with a patch before, let alone a complete patch in a 
> PR.
> 
> 
> There's a few things I'd like to sort out before this is committed.
> 
> 
> I think ORACLE is a better name for the option and plist variable than 
> the rather cryptic OCI8.
> 
> 
> The (EXPERIMENTAL) label in an option means that the EXPERIMENTAL switch 
> is implied by that option. I believe you are declaring Oracle support in 
> FreeRADIUS on FreeBSD to be experimental, but that you don't need 
> EXPERIMENTAL set. If so, I think we need to explain this a different 
> way, rather than overloading the (EXPERIMENTAL) label.
> 
> 
> Can you explain the patch to src/main/exec.c? It would be good to know 
> where it has come from (for example, is it mentioned in the 
> documentation for rlm_sql_oracle, or on bugs.freeradius.org?) and why it 
> is needed. I see what it does, but with FreeRADIUS being a security 
> critical port, I'd rather understand it fully before this is committed.
> 
> I've tried to limit patches in the FreeRADIUS ports to the build system 
> only since I took over maintainership, but sometimes patches to the 
> actual code are necessary, and this seems to be one of those occasions. 
> The Debian OpenSSL debacle stands out in my mind as a demonstration of 
> the danger of single line changes to security critical code, though I 
> realise this patch doesn't touch cryptographic code.
> 
> 
> PORTREVISION should arguably be bumped.
> 
> 
> The patch as it stands is not approved, but I'm sure we can sort out the 
> minor changes to get it into shape. I'm very grateful for your 
> submission and look forward to devising an approved patch together.
> 
> 
> Unfortunately I'm away for most of the next two and a half weeks, but 
> I'll try to keep an eye on this PR via my laptop, especially with the 
> FreeBSD 8.0 ports freeze so close.
> 
> 
> With best wishes,
> 
> 
> 
> 
> David (maintainer, net/freeradius2)
> -- 
> David Wood
> david@wood2.org.uk

-- 
Ryan Steinmetz
Lead Security/Systems Administrator
Infrastructure Engineering
Rochester Institute of Technology
585.475.5663
PGP: EF36 D45A 5CA9 28B1 A550  18CD A43C D111 7AD7 FAF2

Comment 7 Wesley Shields 2009-08-07 00:01:24 UTC

On Thu, Aug 06, 2009 at 10:57:11PM +0100, David Wood wrote:
> PORTREVISION should arguably be bumped.

I only did a quick review of the patch before Ryan submitted it and I
thought about this. The option defaults to off so there is no need to
bump PORTREVISION (it doesn't affect the package and it doesn't fix a
bug in the existing port).

I leave the rest of your comments up to Ryan but want to say thank you
for being responsive and I look forward to concluding this matter.

-- WXS

Comment 8 Wesley Shields 2009-08-24 21:05:52 UTC

On Thu, Aug 06, 2009 at 11:30:07PM +0000, Ryan Steinmetz wrote:
>  Updated patch attached.

David,

Do you have any comments on this updated patch? I'd like to see
PORTREVISION removed for the reason I mentioned earlier: the patch adds
an option which defaults to off so it doesn't affect the default package
or currently installed ports.

This patch has been languishing since August 6th so I'd like to bring it
to a conclusion and get it committed. If you are not comfortable
approving the changes to the code I'd suggest we get one of the
upstream developers to review the proposed changes.

Thanks!

-- WXS

David,

I've added an updated patch that is up to date in light of the libtool/libltdl updates.

I've also added a 'make config' knob for enabling developer mode, which can help developers more-easily debug things using the version of FR in ports.

-r

On (08/24/09 16:05), Wesley Shields wrote:
> On Thu, Aug 06, 2009 at 11:30:07PM +0000, Ryan Steinmetz wrote:
> >  Updated patch attached.
> 
> David,
> 
> Do you have any comments on this updated patch? I'd like to see
> PORTREVISION removed for the reason I mentioned earlier: the patch adds
> an option which defaults to off so it doesn't affect the default package
> or currently installed ports.
> 
> This patch has been languishing since August 6th so I'd like to bring it
> to a conclusion and get it committed. If you are not comfortable
> approving the changes to the code I'd suggest we get one of the
> upstream developers to review the proposed changes.
> 
> Thanks!
> 
> -- WXS

-- 
Ryan Steinmetz
Lead Security/Systems Administrator
Infrastructure Engineering
Rochester Institute of Technology
585.475.5663
PGP: EF36 D45A 5CA9 28B1 A550  18CD A43C D111 7AD7 FAF2

Comment 10 dfilter service 2009-09-09 18:02:44 UTC

wxs         2009-09-09 17:02:31 UTC

  FreeBSD ports repository

  Modified files:
    net/freeradius2      Makefile pkg-plist 
  Added files:
    net/freeradius2/files extra-patch-exec.c 
  Log:
  - Add support for Oracle and debugging options (both off by default).
  
  PR:             ports/137425
  Submitted by:   Ryan Steinmetz <rpsfa@rit.edu>
  Approved by:    maintainer timeout
  
  Revision  Changes    Path
  1.79      +19 -1     ports/net/freeradius2/Makefile
  1.1       +11 -0     ports/net/freeradius2/files/extra-patch-exec.c (new)
  1.40      +5 -0      ports/net/freeradius2/pkg-plist
_______________________________________________
cvs-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/cvs-all
To unsubscribe, send any mail to "cvs-all-unsubscribe@freebsd.org"

Comment 11 Wesley Shields 2009-09-09 18:08:02 UTC

State Changed
From-To: feedback->closed

Committed. Thanks!